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

Clean up graphics mode #123

Closed

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Jun 7, 2020

Extract resetting of the dirty area - use constants, since the actual values are irrelevant, only their relation is important for flush to return early

Remove redundant checks and min/max calls - assuming width/height is always divisible by 8, set_pixel will make sure max_x and max_y is in the valid range

Hi! Thank you for helping out with SSD1306 development! Please:

  • Check that you've added documentation to any new methods
  • Rebase from master if you're not already up to date
    - [ ] Add or modify an example if there are changes to the public API
    - [ ] Add a CHANGELOG.md entry in the Unreleased section under the appropriate heading (Added, Fixed, Changed, etc) No visible changes
  • Run rustfmt on the project with cargo fmt --all - CI will not pass without this step
  • Check that your branch is up to date with master and that CI is passing once the PR is opened

PR description

This needs some double checking because my understanding may be incorrect.

Closes #122 but that is not a bug; may result in a negligible amount of perf gain (some checks per frame).

@jamwaffles
Copy link
Collaborator

I haven't got to the bottom of exactly why yet, but the rtfm_dvd example crashes when the image touches the right hand side of the display. This doesn't occur in master so I'm assuming there's an overflow error introduced in this branch somewhere.

@bugadani
Copy link
Contributor Author

bugadani commented Jun 8, 2020

Well, that means my logic was incorrect somewhere. Thanks, I'll check that. In the mean time, this gets demoted to draft :)

@bugadani bugadani marked this pull request as draft June 8, 2020 09:24
@bugadani
Copy link
Contributor Author

bugadani commented Jun 8, 2020

The collision detection in rtfm_dvd triggers when the right edge of the image is outside of the display area, and that crashes because max_x (or max_y) can get larger than the width/height of the screen. This is because I removed the min calls in flush() and set_pixel does not validate the coordinates - this also means the rendered image could be incorrect in that case, even if I reintroduce the min calls.

Basically, currently it's permitted to draw outside of the screen, as long as it's addressing into the display buffer, so it gets wrapped into a different part of the image. This could be justified for performance reasons IMO, and handled by not trying to draw there in the first place, but that's just my .02€.

@bugadani
Copy link
Contributor Author

bugadani commented Jun 8, 2020

I've reintroduced clipping the display area to its physical size, but the possibility of incorrect drawing is still there in set_pixel.

Extract resetting of the dirty area
@bugadani bugadani marked this pull request as ready for review June 12, 2020 13:28
@bugadani
Copy link
Contributor Author

bugadani commented Jun 12, 2020

I'm closing this in favour of #125 and #124

@bugadani bugadani closed this Jun 12, 2020
@bugadani bugadani deleted the cleanup-graphics branch June 12, 2020 13:38
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.

Incorrect min_y?
2 participants