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

Dragging properties from inspector into code generates code to get the property #8017

Open
theraot opened this issue Oct 7, 2023 · 9 comments · May be fixed by godotengine/godot#82947
Open

Comments

@theraot
Copy link

theraot commented Oct 7, 2023

Describe the project you are working on

None in particular

Describe the problem or limitation you are having in your project

When we drag a property from the inspector to the code editor we get the name of the property between quotes.

However, this ignores the path required to access the property.

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

I want that when I drag a property from the inspector to the code editor while holding CTRL it will generate code to access the property (including node path), and only quoted if necessary:

Examples:

  • property_name
  • $relative/node/path.property_name
  • %unique_name.property_name
  • get("property/name")
  • $relative/node/path.get("property/name")
  • %unique_name.get("property/name")

Note: this is a counter-proposal to #8014

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

When a property is dragged and dropped from the inspector, get this data:

  • the object
  • the name of the property
  • the value

This is the general algorithm I have come up with:

  • Figure out if the property needs to be quoted. If it should, update the property name to its quoted version.
  • If there isn't an edited scene, output the property name. END.
  • Get the node associated with the edited script.
  • If the object we got from the inspector is not a node, output the property name. END.
  • If the node associated with the script is the same we got from the inspector:
    • If we quoted the property output code of the form get(property_name)
    • If we didn't quote it, output code of the form property_name
  • If it is not the same node
    • If the node we got from the inspector has a unique name, the path will be of the form %unique_name
    • If the node does not have a unique name, set the path will be the the path from the node associated with the edited script to the node we got from the inspector.
    • If we quoted the property output code of the form $path.get(property_name) or %unique_name.get(property_name)
    • If we didn't quote it, output code of the form $path.property_name or %unique_name.property_name

The proposed approach has the following limitation: It cannot generate code for a property of a resource in a sub inspector. I want advice on how to implement this. This has been resolved in the pull request.

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

This is a core feature.

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

This is a core feature.

@theraot

This comment was marked as outdated.

@aXu-AP
Copy link

aXu-AP commented Oct 7, 2023

But if I want to set instead of get? While I see the usefulness in this feature (would be handy for working with AnimationTrees or shaders), I'm afraid that the logic for guessing what the user wants is quite complex and it still might be perceived as unpredictable.
Edit: After testing your pr draft, I really do want this 😄

@theraot
Copy link
Author

theraot commented Oct 7, 2023

@aXu-AP No guessing. Either supporting a set or not. To support set we could introduce another modifier key. Yet, the value set would have to be the current value of the property. My current thinking is that could a following proposal and pull request. Perhaps it could be in this one if people really want that.

@timothyqiu
Copy link
Member

Perhaps it could be in this one if people really want that.

People really want that. The original purpose of supporting drag & drop property names into scripts is to make setting AnimationTree parameters easier.

@theraot
Copy link
Author

theraot commented Oct 7, 2023

@timothyqiu In that case help with this, what should be the other modifier?

This is the one we have:

const bool drop_modifier_pressed = Input::get_singleton()->is_key_pressed(Key::CMD_OR_CTRL);

It could be Shift, it could be Alt, it could be something else, and I do not know if there is a guideline for this somewhere.

@aXu-AP
Copy link

aXu-AP commented Oct 7, 2023

I think shift should be a good modifier key. I don't know if there's any guideline anywhere, but in my mind I associate shift for inverting (ie. undo => redo or in this case get => set) and alt for alternate, maybe even not related action.

@timothyqiu
Copy link
Member

timothyqiu commented Oct 7, 2023

Could be Shift. Alt should act as a last resort I believe.

Regarding this proposal and #8014, I doubt if we really want to generate such code snippet. Best practices for engine contributors suggests we should only add solutions for real frequent problems.

For example

  • Dropping files to generate filenames / preloads solves the problem that we can't trigger filename autocompletion. It's also common practice to hold an array of filenames / loaded resources.
  • Dropping node paths is related to autocompletion too, for the same reason.
  • Dropping onready variables definitions solves the problem that we have to repeat these boilerplates if we want to access nodes efficiently.
  • Dropping quoted property path solves the problem that AnimationTree parameters are usually too long to type and no autocompletion is available.

For general properties, I don't know the reason to add such feature. Not knowing the property exists when learning the engine sounds reasonable, but then you won't need this after a few tries.

$relative/node/path.get("property/name")
%unique_name.get("property/name")

Regarding this and nested resources. I find it hard to adapt to something like my_onready_variable.get("property/name"). Typing something and/or dropping quoted property path seems easier:

  • Start typing directly if you want to access property of current node
  • get(|) drop quoted property path
  • nested.resource_or_node.| start typing for autocompletion
  • nested.resource_or_node.get(|) drop quoted property path

@theraot
Copy link
Author

theraot commented Oct 7, 2023

@timothyqiu I can argue that this proposal and all those features you mention do not solve any real problem because you can always type the code. Or, I can argue that it is trying to solve the general problem (so it works for different nodes, unique name, sub resources and so on), which the contributor guidelines recommend against.

In fact, in the past, I have convinced myself against making proposals on similar grounds.

In the case somehow I convinced myself that if it is not desired, then it won't be merged, and that's fine.


Addendum: I'm getting a theme from what you said: autocompletion should be fixed/improved.

@timothyqiu
Copy link
Member

Yeah, the existing drag & drop behavior completes what current autocompletion implementation is not good at :P And onready generation is to alleviate boilerplates.

Since autocompletion works fine on general property access, I think this seems unnecessary. But yeah, this is only my personal opinion. The PR itself seems good.

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.

4 participants