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

Assign value to property by dropping to scene tree #50517

Merged
merged 1 commit into from
Jul 19, 2021

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Jul 16, 2021

I had this idea for a long time and gave it a shot today. Somewhat related to #16464 and #10684.
Basically, you can drop files onto scene tree nodes and they will be assigned to properties.

eCczALT5q2

If you hold Ctrl, a popup will appear with list of available properties. If not, the resource is assigned to the first compatible property.
(see my last comment)

@KoBeWi KoBeWi added enhancement topic:editor usability cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Jul 16, 2021
@KoBeWi KoBeWi added this to the 4.0 milestone Jul 16, 2021
@KoBeWi KoBeWi requested a review from a team as a code owner July 16, 2021 14:38
@jcostello
Copy link
Contributor

Does it works to materials in a mesh for example?

@Calinou
Copy link
Member

Calinou commented Jul 16, 2021

Does it works to materials in a mesh for example?

It should work in theory, the issue is there are 3 locations in which the material can be defined (from top to bottom in the inspector):

  1. In the MeshInstance's Mesh resource itself.
  2. In the MeshInstance's Materials property (Surface Material Override in master). If the mesh has multiple materials, the first surface will likely "win".
  3. In the MeshInstance's GeometryInstance Material Override property.

I think the one that will "win" here is the second one, and this is usually what you want anyway (as changes to the material will be kept after reimporting the mesh).

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 16, 2021

Yes, surface material override is supported, same with the other material override property. You can't only put the resource into mesh, because its material is inside a sub-resource. Dropping on nodes only works with properties of the node, not its sub-resources.

Copy link
Member

@groud groud left a comment

Choose a reason for hiding this comment

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

This feature is a really good idea!

If you hold Ctrl, a popup will appear with list of available properties. If not, the resource is assigned to the first compatible property.

I think this should be made simpler. Instead, I would simply count the number of compatible properties, if there's only one, assign it directly, and if there are more than one, display the list. It's simpler and avoid using a key modifier which might sometimes be hard to discover.

editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 19, 2021

I think this should be made simpler. Instead, I would simply count the number of compatible properties, if there's only one, assign it directly, and if there are more than one, display the list.

Sounds good, but would it be ok if it still used Control, but to bypass the menu instead? So the behavior would be: show list of properties if there is more than 1 or don't show it if there is one property or Ctrl is held. This way it would be still possible to quick-assign something even if there are more slots (and we just assume that the first one is most common to use).

@groud
Copy link
Member

groud commented Jul 19, 2021

This way it would be still possible to quick-assign something even if there are more slots (and we just assume that the first one is most common to use).

Hmm, I am not against the idea. But my issue with it is that I am not sure we kind of respect the fact that the "most expected property" always comes first. So I think it might be simpler to display the list in any case, to avoid frustrating users. As the supposed "most expected property" would be the first entry in the list, it's a simple one click more.

If we don't want to have this one more click, another solution might be to display the list, but if you don't click, or click elsewhere, it would assign it to the first property anyway. In such case, it could display something like "(default)" next to the first property in the list to make it clearer it would be assigned anyway.

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 19, 2021

Ok, removed the Ctrl. The list will appear automatically when there's multiple allowed properties.

@akien-mga akien-mga merged commit b1eee24 into godotengine:master Jul 19, 2021
@akien-mga
Copy link
Member

Thanks!

@KoBeWi KoBeWi deleted the 🌳💣 branch July 19, 2021 16:14
@akien-mga
Copy link
Member

akien-mga commented Jul 20, 2021

Cherry-picked for 3.4.

@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Jul 20, 2021
@akien-mga
Copy link
Member

Cherry-picked for 3.4.

Actually not, there are too many uses of 4.x specific APIs in this commit, it would require a manual backport.

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.

5 participants