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

Add MagnifyGesture to Advanced Import dialog zooming #92235

Merged

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented May 22, 2024

This adds the ability for Macbooks and other devices that use the magnifying gesture to zoom in the advanced import dialog.

Showcase

Bildschirmaufnahme.2024-05-22.um.04.18.43.mov

@paddy-exe paddy-exe requested a review from a team as a code owner May 22, 2024 02:23
@akien-mga akien-mga added this to the 4.x milestone May 22, 2024
@paddy-exe paddy-exe requested a review from fire August 28, 2024 12:46
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

It looks ok; I'm not too fond of magical constants, though, especially in UI.

Can you get the factor to be 0, triggering a divide by 0?

@paddy-exe
Copy link
Contributor Author

It looks ok; I'm not too fond of magical constants, though, especially in UI.

We have the same constants for the MouseButton Scrolling (just above my changes):

Ref<InputEventMouseButton> mb = p_input;
if (mb.is_valid() && mb->get_button_index() == MouseButton::WHEEL_DOWN) {
  (*zoom) *= 1.1;
  if ((*zoom) > 10.0) {
    (*zoom) = 10.0;
  }
  _update_camera();
}
if (mb.is_valid() && mb->get_button_index() == MouseButton::WHEEL_UP) {
  (*zoom) /= 1.1;
  if ((*zoom) < 0.1) {
    (*zoom) = 0.1;
  }
  _update_camera();
}

Can you get the factor to be 0, triggering a divide by 0?

No. I have tested it myself.

Copy link
Contributor

@lyuma lyuma left a comment

Choose a reason for hiding this comment

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

I would feel safer about this if there is either documentation proving it won't be 0, or some check to prevent division by 0.

other than that, @fire 's comment sounds like an approval, and I think it seems good.

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Dec 25, 2024

I would feel safer about this if there is either documentation proving it won't be 0, or some check to prevent division by 0.

other than that, @fire 's comment sounds like an approval, and I think it seems good.

@bruvzg Could you give your insight into this if InputEventMagnifyGesture get_factor can reach zero? If it actually can I would add the extra check.

@bruvzg
Copy link
Member

bruvzg commented Dec 26, 2024

Could you give your insight into this if InputEventMagnifyGesture get_factor can reach zero?

It should not happen with real events. But nothing in the event code prevents it from being zero (you can create an event with zero factor from a script and feed it to the Input.parse_input_event()), so having a check is safer.

@paddy-exe paddy-exe force-pushed the advanced-import-macbook-mouse-magnify branch from 8ece453 to d96ce7e Compare December 26, 2024 22:53
@paddy-exe paddy-exe force-pushed the advanced-import-macbook-mouse-magnify branch from d96ce7e to f1b3f17 Compare December 26, 2024 22:53
@paddy-exe
Copy link
Contributor Author

Check is implemented and PR rebased.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Dec 27, 2024
@Repiteo Repiteo merged commit d8b1a5a into godotengine:master Dec 30, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 30, 2024

Thanks!

@paddy-exe paddy-exe deleted the advanced-import-macbook-mouse-magnify branch January 7, 2025 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants