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

Allow to change the brush-size with the mouse-wheel #271

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

simonbethke
Copy link
Contributor

I implemented a hidden but useful feature that allows to adjust the brush-size with the mouse-wheel by holding down the shift-key.

@simonbethke
Copy link
Contributor Author

simonbethke commented Nov 7, 2024

Actually, this even fixes a bug that is still existent for other selection-modes:

While selecting, you can zoom in and out with the mouse-wheel. I don't think this is how it is supposed to work. Needs to be fixed for rectangle and poly (and lasso) selection too.

And this is the part where I base-class would become handy. I have the impression, that most of my last additions could benefit from the suggested refactoring.

@slimbuck
Copy link
Member

slimbuck commented Nov 7, 2024

Please could you merge main into this PR then see a few changes I prepared here? https://github.com/slimbuck/supersplat/tree/brush-wheel-fixes

@simonbethke
Copy link
Contributor Author

I don't know what exactly you mean by merging main into this PR. What I can see in that PR is, that you are accessing a "shiftKey" flag on a WheelEvent that doesn't have this flag. I had more or less your version of the code and it wasnt working, because of that.

@simonbethke
Copy link
Contributor Author

My suggestion would be, to track the keydown/keyup events in the controller.ts or at any other more global space and query that to know the modifyer-key state.

@slimbuck
Copy link
Member

slimbuck commented Nov 7, 2024

I'm not sure why you wouldn't see shiftKey tbh https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/shiftKey. It's definitely there.

Pressing shift key switches the wheel axis from Y to X using an actual mouse wheel and switches the X/Y axes on a touchpad too. Perhaps you were testing with a touch pad and that's why it appeared to work for you?

If you test your PR with an actual mouse you should also see that deltaX is being sent in the wheel message, no longer deltaY. (On my mac pad I have to swipe left/right not up/down when shift is pressed.

I don't know what exactly you mean by merging main into this PR.

The main branch now has commits that you've not merged into this branch of yours. I was suggesting you merge the latest commits from main branch into this one.

@simonbethke
Copy link
Contributor Author

Ok, I didn't know that the axis are flipped when holding shift.

@slimbuck
Copy link
Member

slimbuck commented Nov 7, 2024

Also, in the updated PR, I use the existing larger/smaller logic which correctly tests radius min/max size (unlike your implementation).

@simonbethke
Copy link
Contributor Author

Thanks for the fixes. I merged main and your branch. Then I tested and found that for my normal mouse in chrome on a windows computer the delta-axis doesn't change when holding shift. So I calculated deltaX + deltaY and worked with that value. There used to be also a property called wheelDelta, but it seems as if it doesn't exsit anymore. Maybe they changed the API a bit and maybe since then, the axis aren't changed anymore by holding shift.

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

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

I like your idea of combing both X and Y.

It's still not working quite right, but I'll address the remaining issues after merge.

@slimbuck slimbuck merged commit d33c4de into playcanvas:main Nov 8, 2024
2 checks passed
@simonbethke simonbethke deleted the mouse-wheel-brush-size branch November 8, 2024 11:30
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.

2 participants