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

Streamline InputEvent class hierarchy to define position property only once #59144

Closed

Conversation

Sauermann
Copy link
Contributor

@Sauermann Sauermann commented Mar 14, 2022

This PR changes the InputEvent class hierarchy, so that the position property is defined only in a single new class named InputEventWithPosition. Current status is, that position gets defined in four different classes with their own getters and setters, which would be simplified by this patch. Another benefit is, that this change makes it easy to distinguish between events with and without position.

In order to make this possible, this patch changes InputEventScreenDrag and InputEventScreenTouch, so that they now also inherit from InputEventWithModifiers.

The changed class hierarchy would look like this:

  • InputEventFromWindow
    • InputEventWithModifiers
      • InputEventKey
      • InputEventWithPosition (new)
        • InputEventScreenDrag
        • InputEventScreenTouch
        • InputEventGesture
        • InputEventMouse

Implements godotengine/godot-proposals#4158

Bugsquad edit: Related to #55300 and #58334

@Sauermann Sauermann requested review from a team as code owners March 14, 2022 16:38
@Calinou Calinou added this to the 4.0 milestone Mar 14, 2022
@akien-mga
Copy link
Member

That makes sense to me. I thought about the same but didn't push on that idea since the screen event didn't inherit InputEventWithModifiers. But changing their inheritance to allow this makes sense.

@@ -7,7 +7,7 @@
<tutorials>
</tutorials>
<members>
<member name="window_id" type="int" setter="set_window_id" getter="get_window_id" default="0">
<member name="window_id" type="int" setter="set_window_id" getter="get_window_id">
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why this is changing in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked myself the same question, but could not find any reason for this.

@Sauermann
Copy link
Contributor Author

Sauermann commented Mar 14, 2022

Two additional thoughts came to my mind:

  • After this change, InputEventWithModifiers is the only class, that inherits directly from InputEventFromWindow. Should they be kept separate?
  • After this change, InputEventGesture is an empty class, that is used only in a few locations within the source code. Would it make sense to remove that class?

@groud
Copy link
Member

groud commented Mar 22, 2022

On my side I am not sure about the usefulness of this.
Making InputEventScreenDrag, InputEventScreenTouch and InputEventGesture extend InputEventWithModifiers does not seems to solve a real-life use case, as tactile input and keyboard, are usually used separately.

Also, I think making a common InputEventWithPosition class is not really useful beside the fact it allows reducing a little bit duplicated code. In general, having the possibility to manipulate objects using such an abstract class should help, but in that case, all are quite different events that will need to be handled very differently anyway. This limits the amount of code you could share for all those events.

To sum up, I believe the gain from this PR is limited while it make the class hierarchy more complex (which is visible to the end user, not only internal).

@Sauermann
Copy link
Contributor Author

Sauermann commented Mar 22, 2022

@groud regarding real-life use cases, I just made an argument here: godotengine/godot-proposals#4158 (reply in thread)

@groud
Copy link
Member

groud commented Mar 22, 2022

Well, from what I read is that his PR would only help to solve an internal implementation small issue. So this is a contributor's problem, not a user's one. So IMO, this is not sufficient to justify a change there.

Also, regarding your issue, you should check for the first class, and not rely on "set_position". It really unlikely we are going to add a lot more of new events with position anyway, so there's no need to change the class hierarchy just to future-proof that.

@Sauermann
Copy link
Contributor Author

This was discussed in the PR-review-meeting.
In the future it may be a reasonable change for combining different mouse-similar event types, however not at the moment.
Because of merge-conflicts I am closing this, since it would have to be rewritten in any case.

@Sauermann Sauermann deleted the proposal-event-position branch January 31, 2023 22:09
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.

None yet

5 participants