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 an option to the project settings for quick testing in “Remote” mode #9873

Open
JekSun97 opened this issue Jun 2, 2024 · 14 comments

Comments

@JekSun97
Copy link

JekSun97 commented Jun 2, 2024

Describe the project you are working on

Simulator, with a large number of nodes

Describe the problem or limitation you are having in your project

I'm going to transfer my old project from version 3 to 4, in this project I often have to use "Remote" to test and change node parameters, in version 3 it was easier, but now in order to find these nodes, I will have to add them to all project functions the second positive argument to add_child(node,true) to quickly find them in the Remote tab. This second argument negatively affects performance, judging by the documentation, which means I will have to rewrite all the add_child functions again, only without the second argument, when testing the project is over, which is naturally time-consuming and inconvenient.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I propose to add an option to the project settings so that we can control this second argument throughout the entire project, if it is more convenient for us to find the scene in the Remote tab by name, we set this checkbox to true in the project settings, and all our nodes are displayed as in version 3, if it is more convenient for us to control this manually, we simply uncheck this option.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

We just need to add one condition before using the add_child function in the GDScript source code.
We check if this option in the project settings is true, we use add_child(...,true,...) everywhere, otherwise we use the normal add_child as it is now, for compatibility with older versions, we can just check if is this option in the project settings or not?

If this enhancement will not be used often, can it be worked around with a few lines of script?

Yes, but if we release the project to production, we will need to rewrite all the add_child functions, convenient? I don't think.

Is there a reason why this should be core and not an add-on in the asset library?

I can’t imagine a situation where we have to set the second argument to true for one node, but not on other nodes, it’s convenient to use it for all nodes at once, in testing, and this should become part of the engine.

@AThousandShips

This comment was marked as outdated.

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

Yes, but if we release the project to production, we will need to rewrite all the add_child functions, convenient? I don't think.

It's very simple to work around this:

extends Object
class_name MyGlobal

static FORCE_READABLE_NAMES = true

And then everywhere you need it:

add_child(my_child, MyGlobal.FORCE_READABLE_NAMES)

No reworks necessary

Or you can even use Engine.is_editor_hint(), which could be used in the MyGlobal class

@JekSun97
Copy link
Author

JekSun97 commented Jun 2, 2024

How do you mean "now"? This hasn't changed since 3.x, what was different in 3.x that made this harder?

We launch these two projects, one for version 3, the other for version 4, their code and actions are absolutely identical, but the result in the “Remote” tab is different (I hope I didn’t confuse you =D)
TestFor4And3.zip

@AThousandShips

This comment was marked as outdated.

@timothyqiu
Copy link
Member

FYI: Changing the @ name scheme from using node name to node type was done since 4.1 by godotengine/godot#75760

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

Oh then that part is relevant, my bad it wasn't mentioned in the issue report that that was the difference (it was unclear due to the mistakes in the original issue with the name)

@JekSun97
Copy link
Author

JekSun97 commented Jun 2, 2024

Can you show the difference in screenshots to show that it works differently because it looks like it works exactly the same
remote
(What's strange is that I just noticed that for some reason the first node looks the same as in version 3)

FYI: Changing the @ name scheme from using node name to node type was done since 4.1 by godotengine/godot#75760

Oh, this is naturally a great deliverance, now it’s clear why this is so

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

Thank you for the screenshots with the mistake fixed, confirmed it in 4.x as well, now I'd say that an option would be to control how it's named, but it's so very simple to just make it use readable names directly if you need them (see above)

What's strange is that I just noticed that for some reason the first node looks the same as in version 3

That's not strange it's just that it doesn't collide with the name, as the documentation clarifies

But as mentioned on the issue:

  • Using a project setting would hurt performance, as it'd have to be checked every time this method is called, unless it'd be static somewhere but that's difficult to have in a simple place and be loaded once (on startup)
  • It'd be confusing to have the argument be ignored (or flipped) by a project setting

@JekSun97
Copy link
Author

JekSun97 commented Jun 2, 2024

  • Using a project setting would hurt performance, as it'd have to be checked every time this method is called, unless it'd be static somewhere but that's difficult to have in a simple place and be loaded once (on startup)

By the way, if there is already a second argument in add_child, it turns out that this costly condition for performance is already present in the GDScript, my logic is to simplify the use of this argument, but thereby losing control over the individual nodes that need it or don’t need it (second argument)

Because I don’t see the point of the second argument, unless you start writing your project with a method like you described above

extends Object
class_name MyGlobal

static FORCE_READABLE_NAMES = true

add_child(my_child, MyGlobal.FORCE_READABLE_NAMES)

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

it turns out that this costly condition for performance is already present in the GDScript

What do you mean? What costly performance? An argument is not the same as loading a setting, and removing it would break compatibility. We should not remove the argument, because that'd break a lot of stuff with compatibility, so if there is a project setting it would control how the argument is interpreted

Because I don’t see the point of the second argument, unless you start writing your project with a method like you described above

Why would it be strange to use it conditionally? It's pretty obvious to me that you might want it some times and not some times? Right?

@JekSun97
Copy link
Author

JekSun97 commented Jun 2, 2024

What do you mean? What costly performance? An argument is not the same as loading a setting

Doesn't the add_child function use a condition to determine what to do based on its second argument?

and removing it would break compatibility. We should not remove the argument, because that'd break a lot of stuff with compatibility

I understand that this is not an easy solution, unless we consider it for future versions, where the compatibility of many functions will already be broken

@AThousandShips
Copy link
Member

AThousandShips commented Jun 2, 2024

Doesn't the add_child function use a condition to determine what to do based on its second argument?

Yes but it's different from reading a project setting, conditionality is very different

The chagne to compatibility would not happen until 5.0, and wouldn't be something to change IMO, there's no real reason to make it not something you can control by case basis, and just a setting, that's strange IMO

@JekSun97
Copy link
Author

JekSun97 commented Jun 2, 2024

Got it, thanks for participating in the discussion )

@DanielSnd
Copy link

I can’t imagine a situation where we have to set the second argument to true for one node, but not on other nodes, it’s convenient to use it for all nodes at once, in testing, and this should become part of the engine.

The readable names are essential for nodes that are synchronized over networking. The RPCs, MultiplayerSpawner and MultiplayerSynchronizer in godot require the nodepaths to be the same across the network. So in that case you'd want for the second argument to be true for those nodes, and not for others.

The static variable workaround should work fine for your use-case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants