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

Rename InputFilter singleton back to Input #639

Closed
1 task
Xrayez opened this issue Mar 26, 2020 · 18 comments · Fixed by godotengine/godot#38295
Closed
1 task

Rename InputFilter singleton back to Input #639

Xrayez opened this issue Mar 26, 2020 · 18 comments · Fixed by godotengine/godot#38295
Milestone

Comments

@Xrayez
Copy link
Contributor

Xrayez commented Mar 26, 2020

Context: Input singleton is renamed to InputFilter in godotengine/godot#37317.

Important Note: the validity of this proposal is partly determined by the resolution of godotengine/godot#37319 (aka bug or feature?), see also #639 (comment).

Describe the project you are working on:
A project heavily relying on input handling, manipulation, recording, and replication of input states, being one of the key links to project's architectural prosperity.

Describe the problem or limitation you are having in your project:
I have lots of scripts relying on existing Input references throughout my project (and several other experimental/test projects). Typing InputFilter makes it difficult to write code quickly for such a commonly used singleton, worsens the readability of the existing codebase. Also see limitations in a counter-proposal below.

Describe the feature / enhancement and how it helps to overcome the problem or limitation:
I present two options to consider, the first being the required one to fulfill this proposal, and the second is optional and builds upon the renaming rationale.

1. Renaming InputFilter singleton back to Input

While I completely understand the rationale behind the renaming:

  1. The singleton mostly filters input events.
  2. Explicit is better than implicit (when there's no better option).

Despite this, the InputFilter name only adds unnecessary complexity and information overload for something which can be easily looked up in the Input documentation instead.

In fact, some of the methods cannot even be considered as something pertaining to "filtering" concept:

InputFilter.action_press()
InputFilter.action_release()
InputFilter.get_connected_joypads()
InputFilter.get_current_cursor_shape
InputFilter.get_mouse_mode
InputFilter.start_joy_vibration
InputFilter.warp_mouse_position
# ...
# etc.

(2) Introducing input filtering as a built-in feature

The rationale behind renaming is quite legit, but I think we can build upon this to further improve the idea. Some of you may have read a proposal I made roughly 6 months ago:

The proposed InputState class can be seen as a input filtering layer of the input polling system.

In a way, InputState == InputFilter, because it does replicate the same Input API.

In there, I show how it's possible to treat the input state as something which can be passed around and shared between multiple instances. The Input singleton would always have a master InputState instance as a property.

Compare:

InputFilter.is_action_just_pressed("jump")
# vs
Input.state.is_action_just_pressed("jump")

This requires just the same amount of keystrokes, with the added benefit of having ability to mix, modify, share, serialize, replicate, and simulate input behavior via InputState API.

Not only that, it's also possible to do this:

var input: InputState

func _ready():
    input = Input.state

func _physics_process():
    if input.is_action_just_pressed("jump"):
        pass # jump

And talking about more input filtering behavior:

Input.state.blocking = true
# Prints "False" even if pressed, because the input state is blocked.
print(Input.state.is_action_just_pressed("jump"))

That way, you could control different group of characters, for instance (because it's possible to have multiple InputStates, currently Input provides only one).

Not saying that we should (re)move all Input API to InputState because most users would still prefer Input.is_action_pressed() style I guess, that's still up to discussion. I'm just presenting alternative solutions.

Describe how your proposal will work, with code, pseudocode, mockups, and/or diagrams:

find . -type f -not -path '*/\.*' -exec sed -i 's/InputFilter/Input/g' {} +

and optionally implementing #104 and a draft as seen in godotengine/godot#35240.

If this enhancement will not be used often, can it be worked around with a few lines of script?:
As most changes in the 4.0, this still requires some refactoring in existing projects as well, so more or less yes. But does this change really justify the added usability and readability issues to existing projects?

Also, there's no actual way to use some kind of aliases in GDScript as in other languages. It's also possible to create an Input global class_name script to act as a wrapper over InputFilter singleton, but I don't think it's worth it.

I would personally prefer if we could implement #104, at least to some extent.

Is there a reason why this should be core and not an add-on in the asset library?:
It's possible to create a plugin which could provide the aforementioned Input global class with static methods that use InputFilter under the hood, but the burden of having to go to the asset library in hopes that you find such a utility class far outweighs a relatively trivial change in core.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 26, 2020

Typing InputFilter makes it difficult to write code quickly for such a commonly used singleton

In 99% cases you can just do inp -> Enter (or inpf), so it doesn't really affect typing.

@TheDuriel
Copy link

There really isn't much need to panic over a rename.

Counter proposal: Wrap the new system behind the Input name and call it a day.

@Zylann
Copy link

Zylann commented Mar 26, 2020

Wasn't it just the internal class that was renamed? The "singleton name" is still Input, see https://github.com/godotengine/godot/pull/37317/files#diff-8a4ac1e32d20c004622182f2633f9937L266-R266

I don't know if the singleton name is actually used for the script API tho
Otherwise I agree that this change [renaming Input to InputFilter] doesn't help anything other than wanting expliciteness (for something that didnt really matter anyways)

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 26, 2020

I confirm that one can use Input in scripts, but interestingly it's not highlighted, reported a bug: godotengine/godot#37319.

InputFilter is highlighted though, so something tells me that the rename might not be complete yet, I hope I'm wrong on this one. 🙂

Still, if it's just an internal rename, I wouldn't really want to have inconsistency between C++ and GDScript yet again, only to be changed in subsequent versions.

But if you were to choose between Input and InputFilter name, what would be the answer? 😛

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 26, 2020

One of the major motivations of me writing this is not be the rename itself (though I'm still not happy with this myself). I've chosen a perfect time to consider implementing #104 as a counter-proposal to this. It's been 6 month since I created that proposal and would like to receive more feedback on it, even negative, because to be honest, I'm blocked by lack of review/decision regarding #104, please understand my situation. I understand that the proposal is lengthy but I've tried my best, thank you.

@reduz
Copy link
Member

reduz commented Mar 26, 2020

@Xrayez the way input is handled changed a bit now, as it no longer goes via main loop, plus Input is now only a filter.

There has been a lot of discussion regarding your proposal, but honestly in most cases its easy to solve by just doing something like:

if (Input.is_action_pressed("jump"+str(player)):

It's not pretty but it's not terrible either. There has been discussions about doing something like:

if (Input.is_action_pressed("jump",player):

but its making the system considerably more complex where the existing workaround is ok.

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 26, 2020

@reduz it's not really about multiple players, it's about multiple characters controlled by multiple players, so imagine how this can multiply complexity, I wouldn't create proposal otherwise. I have a notion of player, team, squad, and individual characters per scene.

@reduz
Copy link
Member

reduz commented Mar 26, 2020

@Xrayez again, most of your original proposal are rare corner cases that you can work around creating a system of your own, it does not merit making the input system more complex for this.

@reduz
Copy link
Member

reduz commented Mar 26, 2020

@Xrayez check rule #3, the problem has to be either complex or frequent. In this case, it's neither:

https://docs.godotengine.org/en/stable/community/contributing/best_practices_for_engine_contributors.html

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 26, 2020

@reduz, according to the linked rule:

In our experience, in most cases it’s usually obvious to tell when a problem is complex or frequent, but cases may arise where drawing this line is difficult. This is why discussing with other developers (next point) is always advised.

I want to draw a clear line by discussing this with other developers. I believe the problem is complex enough, but I cannot possibly tell how frequent this problem is to other users (the only way to tell is by number of thumb ups and feedback, and according to that it's more or less frequent problem IMO). How many thumb ups are actually needed to tell whether the problem is frequent?

(to readers, we are talking about #104).

@groud
Copy link
Member

groud commented Mar 26, 2020

How many thumb ups are actually needed to tell whether the problem is frequent?

The number of thumbs up doesn't matter that much, it depends more on how many people explain having a similar problem, which is not at all represented correctly by thumbs up.

Also, as your talked about this proposal on social networks, this also biases the thing, as people just wanting to be supportive might have thumbed up the issue too (I noted that from the list of the people who supported this issue, 70% of them are people I have never seen interacting with the repository before).

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 26, 2020

@groud

The number of thumbs up doesn't matter that much, it depends more on how many people explain having a similar problem, which is not at all represented correctly by thumbs up.

While I agree that it's beneficial for people to present their own use cases supporting the proposal, but based on this preconceived notion, then (as an example) we can consider that presidential election to be absurd. Do you really have to ask each voter regarding his specific opinion about the candidate?

What if the opinion is almost the same, do the voter has to duplicate the proposal text? A thumb up is enough in those cases. I also doubt that core devs would be willing to read all the use cases, especially if they describe them in great detail.

70% of them are people I have never seen interacting with the repository before).

Do these 70% of people are part of the community or just random people, what do you think? Those people have at least an account on GitHub to cast a vote, but even then I think it's not fair to disregard those who actually use the engine but never interact with any part of engine development community.

If this the case, then I'm sorry but this an implementation of autocracy.

@groud
Copy link
Member

groud commented Mar 26, 2020

Do you really have to ask each voter regarding his specific opinion about the candidate?

Well, if you want my opinion, yes, simple elections are absurd. And better systems should ask you a lot more than a single name on a paper. But that's kind of off-topic.

do the voter has to duplicate the proposal text? A thumb up is enough in those cases.

It is definitely not enough. Not a single other user explained the problem they faced in the issue, and Godot's development is lead by problems, not by solutions. So a thumb up isn't worth a lot if people don't explain what problem they faced (and eventually how the proposal would solve it).

Edit: See rule #1 for that.

If this the case, then I'm sorry but this an implementation of autocracy.

From what I remember, we called the Godot development a "do-ocracy", where the ones who contributed to the engine decide. Thus having the support of users only is definitely not enough.
So well, if your question is whether or not Godot is managed as a democracy, the answer is: it's not. :)

@Xrayez
Copy link
Contributor Author

Xrayez commented Mar 26, 2020

@groud thanks for your honest response. It would be good if this could be written in the official documentation pages, as suggested in #575, or even the existing Best practices for engine contributors. 🙂

@groud
Copy link
Member

groud commented Mar 26, 2020

Just to precise, when I said "the ones who contributed to the engine decide", I meant all the people getting involved in the project. That includes a little bit more than programmers of course. :)

I remember we had a few blog post about the project management, like this one, but I agree this should be written down in the documentation, so that it makes things clear.

However I remembered and found this twitter discussion, that explains the philosophy quite well.

@aaronfranke
Copy link
Member

aaronfranke commented Mar 26, 2020

I think the InputFilter name is being unnecessarily explicit and therefore it's not a good change, unless we have some other class relating to inputs.

Related: I also think that PackedVector2Array etc should be renamed to Vector2Array etc, since we only have one of each typed array, it's unnecessary to be explicit with "Packed".

@KoBeWi In 99% cases you can just do inp -> Enter (or inpf), so it doesn't really affect typing.

I disagree with this logic, we shouldn't rely on good editors to have easy reading/writing of code.

This is also a reason why I dislike auto in C++ and var in C# - you can find out the type by hovering over the variable name, but this isn't possible without an IDE (such as viewing diffs on GitHub)

@TheDuriel Counter proposal: Wrap the new system behind the Input name and call it a day.

Then the C++ code and its GDScript front-end are inconsistent with each other...

@TokisanGames
Copy link

How many thumb ups are actually needed to tell whether the problem is frequent?

The number of thumbs up doesn't matter that much, it depends more on how many people explain having a similar problem, which is not at all represented correctly by thumbs up.

People are actively discouraged from commenting if they have nothing new to contribute to a discussion and to use the thumbs up. But if thumbs up are not a useful indication this sends a very confusing, mixed message. On my lighting defaults proposal there are over 100 thumbs up and other emojis, yet the number of commenter is maybe 10. I believe the thumbs up, not the number of commenters.

@groud
Copy link
Member

groud commented Mar 27, 2020

@tinmanjuggernaut well thumbs up is an indicator indeed, but what really matters is what the issue about.

What is important is what those thumbs up mean, which usually depend on the content of the issue. Your issue was more about a problem than a solution, which means that thumbs up are more likely to be from people who agree with the problem first. When the problem or use cases are unclear, your don't know if the thumbs up are for the problem or just because the "proposal sounds cool". So in that case, it's required that people explain their problem or use cases in comments.

But to make it clear, thumbs up are not ignored, they may be useful depending on the context.

akien-mga added a commit to akien-mga/godot that referenced this issue Apr 28, 2020
It changed name as part of the DisplayServer and input refactoring
in godotengine#37317, with the rationale that input no longer goes through the
main loop, so the previous Input singleton now only does filtering.

But the gains in consistency are quite limited in the renaming, and
it breaks compatibility for all scripts and tutorials that access
the Input singleton via the scripting language. A temporary option
was suggested to keep the scripting singleton named `Input` even if
its type is `InputFilter`, but that adds inconsistency and breaks C#.

Fixes godotengine/godot-proposals#639.
Fixes godotengine#37319.
Fixes godotengine#37690.
@mhilbrunner mhilbrunner added this to the 4.0 milestone Apr 29, 2020
akien-mga added a commit to godotengine/godot-platform-haiku that referenced this issue Jun 20, 2020
It changed name as part of the DisplayServer and input refactoring
in #37317, with the rationale that input no longer goes through the
main loop, so the previous Input singleton now only does filtering.

But the gains in consistency are quite limited in the renaming, and
it breaks compatibility for all scripts and tutorials that access
the Input singleton via the scripting language. A temporary option
was suggested to keep the scripting singleton named `Input` even if
its type is `InputFilter`, but that adds inconsistency and breaks C#.

Fixes godotengine/godot-proposals#639.
Fixes #37319.
Fixes #37690.
octetdev2 pushed a commit to octetdev2/godot-tile-module that referenced this issue Apr 24, 2022
It changed name as part of the DisplayServer and input refactoring
in #37317, with the rationale that input no longer goes through the
main loop, so the previous Input singleton now only does filtering.

But the gains in consistency are quite limited in the renaming, and
it breaks compatibility for all scripts and tutorials that access
the Input singleton via the scripting language. A temporary option
was suggested to keep the scripting singleton named `Input` even if
its type is `InputFilter`, but that adds inconsistency and breaks C#.

Fixes godotengine/godot-proposals#639.
Fixes #37319.
Fixes #37690.
akien-mga added a commit to godotengine/godot-visual-script that referenced this issue Aug 23, 2022
It changed name as part of the DisplayServer and input refactoring
in #37317, with the rationale that input no longer goes through the
main loop, so the previous Input singleton now only does filtering.

But the gains in consistency are quite limited in the renaming, and
it breaks compatibility for all scripts and tutorials that access
the Input singleton via the scripting language. A temporary option
was suggested to keep the scripting singleton named `Input` even if
its type is `InputFilter`, but that adds inconsistency and breaks C#.

Fixes godotengine/godot-proposals#639.
Fixes #37319.
Fixes #37690.
edg1000 pushed a commit to edg1000/https-github.com-godotengine-godot that referenced this issue Apr 23, 2024
It changed name as part of the DisplayServer and input refactoring
in #37317, with the rationale that input no longer goes through the
main loop, so the previous Input singleton now only does filtering.

But the gains in consistency are quite limited in the renaming, and
it breaks compatibility for all scripts and tutorials that access
the Input singleton via the scripting language. A temporary option
was suggested to keep the scripting singleton named `Input` even if
its type is `InputFilter`, but that adds inconsistency and breaks C#.

Fixes godotengine/godot-proposals#639.
Fixes #37319.
Fixes #37690.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants