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

Fixed macro definition bug in mouse_c.h #697

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xuehaonan27
Copy link

Please provide Issues links to:

Provide test code:

No test code needed.

Description

screengrab_c.h has a pointer implicit conversion behavior at line 93 (from void* to uint8_t *).
I added an explicit conversion to it.

@xuehaonan27 xuehaonan27 requested a review from vcaesar as a code owner October 8, 2024 03:13
@CLAassistant
Copy link

CLAassistant commented Oct 8, 2024

CLA assistant check
All committers have signed the CLA.

Inside `mouse/mouse_c.h`: line 101
The expansion of MOUSE_COORD_TO_ABS will result in undesired calculation.
@xuehaonan27 xuehaonan27 changed the title Fixed pointer implicit conversion behavior Fixed macro definition bug in mouse_c.h Oct 10, 2024
@xuehaonan27
Copy link
Author

I found a new bug in mouse_c.h.
In line 101 definition of macro MOUSE_COORD_TO_ABS:

#define MOUSE_COORD_TO_ABS(coord, width_or_height) ( \
	((65536 * coord) / width_or_height) + (coord < 0 ? -1 : 1))

It's obvious that coord and width_or_height are not wrapped by brackets.
But follow code:

int32_t x = MOUSE_COORD_TO_ABS(point.x - rect.origin.x, rect.size.w);
int32_t y = MOUSE_COORD_TO_ABS(point.y - rect.origin.y, rect.size.h);

The desired behavior is 65536 * (point.x - rect.origin.x).
But actually it's 65536 * point.x - rect.origin.x.
It's a big bug that would lead to mouse position problem as mentioned in issue #479 and issue #631 .

I don't know whether my fix could solve them completely, but it's definitely a big step forward.

@xuehaonan27
Copy link
Author

Would you review this PR soon? @vcaesar

@PekingSpades
Copy link

Very helpful for my project

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