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

Incorrect min_y? #122

Closed
bugadani opened this issue Jun 6, 2020 · 5 comments · Fixed by #125
Closed

Incorrect min_y? #122

bugadani opened this issue Jun 6, 2020 · 5 comments · Fixed by #125

Comments

@bugadani
Copy link
Contributor

bugadani commented Jun 6, 2020

Based on this metric I believe the linked line may be incorrect.

https://github.com/jamwaffles/ssd1306/blob/96170fd21419e73fdb2299f395b07836d52d090f/src/mode/graphics.rs#L161

If that's supposed to be height, I'm happy to fix it, otherwise please provide an explanation why it's width.

@jamwaffles
Copy link
Collaborator

Yeah that definitely looks like it should be height - 1. There's a similar block of code here, so it might be worth moving the min/max reset into a private method.

@bugadani
Copy link
Contributor Author

bugadani commented Jun 7, 2020

I haven't given it much thought, but shouldn't these be rotation aware values? Is min_y the same when rotated by 90 or 270 degrees?

@therealprof
Copy link
Collaborator

Yes, there're the other variables which take the rotation into account. However it's a bit weird to have the manual comparison in the code. There're max() and min() functions for that.

@bugadani bugadani mentioned this issue Jun 7, 2020
4 tasks
@bugadani
Copy link
Contributor Author

bugadani commented Jun 7, 2020

There's a similar block of code here, so it might be worth moving the min/max reset into a private method.

You mislead me :) That block is marking the whole display to be updated, the original block resets the area to nothing.

@jamwaffles
Copy link
Collaborator

Oop yes you're right, min/max are swapped there. My bad!

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 a pull request may close this issue.

3 participants