Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Image setDelay bugfix #6

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

ascholerChemeketa
Copy link

Existing image code has an assumption that no more than 2 lines will need to be redrawn.
If updateInterval > 2 * width that will not be the case and the image ends up with banding.
This allows for an arbitrary number of lines to be redrawn at each repaint.

It also removes what I believe to be an extraneous condition:
else if (self.lasty + self.updateInterval >= self.height)
From what I can see, either we are in a row and going to stay in that row (the else), or we are going to finish a row and possibly wrap to a new row (first if).

@bnmnetp
Copy link
Member

bnmnetp commented Mar 1, 2024

I don't remember my logic from way back then. My guess is I was trying to minimize the number of pixels I was pushing around. But I wonder if today it would not be simpler to just redraw the image from the beginning to the current location at each interval?

But, if this works, then I'm also fine with just merging it too.

@ascholerChemeketa
Copy link
Author

There does not appear to be much speed up to the partial redraw.

Latest commit just redraws the whole image - doesn't even bother to only redraw up to current location. It seems to run at about the same speed.

If you want simple code path, use the last commit.
If you want to keep the potential optimization, use the previous one.

@bnmnetp
Copy link
Member

bnmnetp commented Mar 1, 2024

I think at this point the simple codepath is better.... Nearly a decade of Moore's law and better javascript engines make things easier.

@bnmnetp
Copy link
Member

bnmnetp commented Mar 1, 2024

I'll try to get this merged later today and get a new release done before rebuilding tomorrow.

@bnmnetp bnmnetp merged commit 399aa8d into RunestoneInteractive:master Mar 4, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants