Skip to content

Conversation

@thomaseding
Copy link
Contributor

  • Use actual Lua class syntax. (I finally read up on Lua 🎉)
  • Avoids recording duplicate image sequences.

@sprngr sprngr self-assigned this May 3, 2022
@sprngr sprngr self-requested a review May 3, 2022 17:16
-- * pollutes "recent files"
-- Anyway, this seems more than fast enough for human inputs. I tried lots
-- of very very fast changes on large detailed canvases, had no issues, and
-- have a computer that is average by 2022 standards.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill be sure to beat this up as I have a computer considered average by 2012 standard.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Ran through a gambit of:

  • Image creation
  • cropping
  • painting
  • fills
  • moving
  • resizing

@thomaseding
Copy link
Contributor Author

thomaseding commented May 4, 2022

Eeep!! I forgot to close the files I opened. Also check if any other potential file open call sites.

EDIT: Done

@thomaseding thomaseding force-pushed the teding/feat/compression branch from 60ae3f9 to 8d4d409 Compare May 4, 2022 20:06
end

local path = get_recording_image_path_at_index(snapshot, 0)
local path = snapshot:get_recording_image_path_at_index(0)
Copy link
Owner

@sprngr sprngr May 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
local path = snapshot:get_recording_image_path_at_index(0)
local path = snapshot:get_recording_image_path(0)

Method used is the old name, needs to be replaced with new one.

Comment on lines 200 to 205
-- NOTE(teding): Unfortunately `Image { fromFile = path }`:
-- * causes load popups
-- * pollutes "recent files"
-- Anyway, this seems more than fast enough for human inputs. I tried lots
-- of very very fast changes on large detailed canvases, had no issues, and
-- have a computer that is average by 2022 standards.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- NOTE(teding): Unfortunately `Image { fromFile = path }`:
-- * causes load popups
-- * pollutes "recent files"
-- Anyway, this seems more than fast enough for human inputs. I tried lots
-- of very very fast changes on large detailed canvases, had no issues, and
-- have a computer that is average by 2022 standards.

Can prune these comments from the code, is preferable to maintain these types of artifacts in PR comments for history tracking.

Comment on lines 192 to 194
-- This intentionally does not do anything "smart" such as scanning the
-- directory if gaps exist because that is O(N) at IO speeds and on every
-- snapshot (where N is the current snapshot index).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-- This intentionally does not do anything "smart" such as scanning the
-- directory if gaps exist because that is O(N) at IO speeds and on every
-- snapshot (where N is the current snapshot index).

Can prune these comments from the code, is preferable to maintain these types of artifacts in PR comments for history tracking.

@sprngr sprngr added the enhancement New feature or request label May 7, 2022
Copy link
Owner

@sprngr sprngr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran through a bunch of testing on my end, performance seems good. Just a few things that need to be addressed including an incorrect method reference.

@sprngr
Copy link
Owner

sprngr commented May 13, 2022

@thomaseding Thanks for updating. Going to pull it down later today and do another pass through.

@sprngr sprngr self-requested a review May 14, 2022 02:37
Copy link
Owner

@sprngr sprngr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomaseding Everything looks good to me. Going to merge it in and get a release tagged and prepare an update to go out to itch. Thank you for contributing :shipit:

@sprngr sprngr merged commit c9f644b into sprngr:master May 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants