-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix #9393: Consistent _buttonDown usage in ScreenSpaceEventHandler.js #12613
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for the pull request, @ayushd150! Welcome to the Cesium community! In order for us to review your PR, please complete the following steps:
Review Pull Request Guidelines to make sure your PR gets accepted quickly. |
I have signed the CLA in Google Forms |
Added myself in the Contributors.md file |
Thanks @ayushd150! First things first, could you let us know the reason for the change? It looks like this may be a fix for #9393, is that correct? If we do want to move forward with this PR–
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @ayushd150. I had a few comments about this approach, see below.
It also seems like you have a bunch of package.json
and formatting changes on this branch. I'm assuming these just got added by mistake but please resolve/remove them to focus only on the code changes made.
Also you can include the updates to @ggetz beat me to it 😆CONTRIBUTORS.md
in this pr instead of a separate one.
this._buttonDown = { | ||
LEFT: false, | ||
MIDDLE: false, | ||
RIGHT: false, | ||
0: false, // LEFT | ||
1: false, // MIDDLE | ||
2: false, // RIGHT | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this initialization should still rely on the values from MouseButton
so they're consistent. You can set the properties dynamically using the enum constants.
this._buttonDown = {
[MouseButton.LEFT]: false,
[MouseButton.MIDDLE]: false,
[MouseButton.RIGHT]: false,
}
if (screenSpaceEventHandler._buttonDown[MouseButton.LEFT]) { | ||
if (screenSpaceEventHandler._buttonDown[0]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is functionally equivalent but harder to read. If you check the initialization of MouseButton
on line 45 you can see the values are already integers. We should use that enum/constants everywhere else for readability. Please undo the changes like this one.
Changes made to _buttonDown in ScreenSpaceEventHandler.js