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

Fix RenderClxOutline OOB Draw #7429

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

kphoenix137
Copy link
Collaborator

@kphoenix137 kphoenix137 commented Sep 24, 2024

Credit to @StephenCWills

Maybe we're not drawing the item CEL in the right spot. Actually.. that might be exactly what the problem is.
If the graphic is 84 pixels tall, and you draw it with the top-left corner at (0,0), then you'd expect it to place pixels between y-coordinates 0 and 83 inclusive.
But CELs draw from the bottom-left. So drawing at (1,85) suggests you assume it won't be including y-coordinate 85, but that doesn't make sense. It would probably then occupy y-coordinates between 2 and 85, not 1 and 84.

@kphoenix137 kphoenix137 reopened this Sep 24, 2024
@kphoenix137 kphoenix137 changed the title Fix RenderClxOutline OOB Fix RenderClxOutline OOB Draw Sep 24, 2024
Copy link
Member

@StephenCWills StephenCWills left a comment

Choose a reason for hiding this comment

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

Sneaky off-by-one errors. CELs drawing from the bottom-left hurts my brain.

@kphoenix137
Copy link
Collaborator Author

Sneaky off-by-one errors. CELs drawing from the bottom-left hurts my brain.

We do have a function that instead draws them from the top left. Might be a worthwhile venture to convert all these drawing calls to that?

@AJenbo AJenbo merged commit d7b5a0c into diasurgical:master Sep 24, 2024
23 checks passed
@kphoenix137 kphoenix137 deleted the fix-RenderClxOutline-OOB branch September 24, 2024 11:52
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.

4 participants