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 vector value linking #59125

Merged
merged 1 commit into from
Jun 14, 2022
Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Mar 14, 2022

Closes godotengine/godot-proposals#144



Linked values change proportionally (unless any of them is 0). The link button only appears if property has PROPERTY_HINT_LINK (for now only Node2D/3D/Control scale).

The linked/unlinked icon is a placeholder, I did my best :/ I'll optimize them when they are replaced.

On a side note, I'm thinking about adding possibility to link 2 properties. This would be mostly useful for min/max values in ParticlesMaterial. So I wonder if the new property hint could be reused for that in the future.

@KoBeWi KoBeWi added enhancement feature proposal topic:editor usability cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Mar 14, 2022
@KoBeWi KoBeWi added this to the 4.0 milestone Mar 14, 2022
@KoBeWi KoBeWi requested review from a team as code owners March 14, 2022 01:14
@fire-forge
Copy link
Contributor

This looks great! However, I think the link button should be moved to the right side as it'd make more sense there than on the left.

Also, are you considering doing this for Vector3 as well? The same argument about scale usually being the same on all axes can be applied to 3D too.

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 14, 2022

Also, are you considering doing this for Vector3 as well?

Not in this PR. I didn't want it too big and we still need to agree on the design of this feature.

@akien-mga
Copy link
Member

On a side note, I'm thinking about adding possibility to link 2 properties. This would be mostly useful for min/max values in ParticlesMaterial. So I wonder if the new property hint could be reused for that in the future.

This is actually possible already, see ADD_LINKED_PROPERTY in object.h.

@groud
Copy link
Member

groud commented Mar 29, 2022

As discussed on a the PR meeting, the PR is very good but would need small changes:

  • The link button should be hidden when the hint is not here. Later on, if we find out the there are situations where the link would be useful but off by default, we could add another hint to specify the default value. But for now there is no need to have them for all Vector2.
  • I think it might depend on how it looks like, but removing the button will make the x/y components not aligned from one property to another. As a consequence it would be interesting to try moving it the right, to see if it looks better.

As a side note (that was not discussed in the PR meeting), do you think you could make the same changes for Vector3/Vector3i too?

@KoBeWi
Copy link
Member Author

KoBeWi commented Mar 29, 2022

This is actually possible already, see ADD_LINKED_PROPERTY in object.h.

This is for dependent properties, it doesn't allow for conditional linking (like with the link button).

As a side note (that was not discussed in the PR meeting), do you think you could make the same changes for Vector3/Vector3i too?

Yes, I was already planning to do it, just waited for the feature approval before putting more work.

@KoBeWi KoBeWi force-pushed the link_to_the_vector branch from cd9fde7 to 6fb3c39 Compare April 4, 2022 23:37
@KoBeWi KoBeWi requested a review from a team as a code owner April 4, 2022 23:37
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 4, 2022

Ok, reworked a bit. Moved the icon to the right and made the button hidden without the link hint:
image
I'll now add it to Vector3.

EDIT:
Done
godot windows tools 64_THCIZ39tQM
It needs to keep 6 ratio values. Also the code duplication in there is ridiculous :/

Aside from icons that need cleanup (and I mean visual cleanup), should be ready to go.

@KoBeWi KoBeWi force-pushed the link_to_the_vector branch from 6fb3c39 to ebf4250 Compare April 5, 2022 00:55
@KoBeWi KoBeWi requested a review from a team as a code owner April 5, 2022 00:55
@KoBeWi KoBeWi force-pushed the link_to_the_vector branch from ebf4250 to 8b99365 Compare April 5, 2022 00:58
@KoBeWi KoBeWi changed the title Add Vector2 value linking Add vector value linking Apr 5, 2022
@KoBeWi KoBeWi force-pushed the link_to_the_vector branch from 8b99365 to 66acf48 Compare April 6, 2022 17:05
@KoBeWi
Copy link
Member Author

KoBeWi commented Apr 6, 2022

Ok added new icon contributed by @redlamp:

The linked icon he provided was the same as Instance icon, so I reused Instance icon. It could be renamed to Linked maybe, but not in this PR.

@KoBeWi KoBeWi force-pushed the link_to_the_vector branch 2 times, most recently from 2d336c4 to 4799ee4 Compare April 6, 2022 17:30
Comment on lines +1871 to +1887
if (spin[0]->get_value() != 0 && spin[1]->get_value() != 0) {
ratio_yx = spin[1]->get_value() / spin[0]->get_value();
ratio_zx = spin[2]->get_value() / spin[0]->get_value();
ratio_xy = spin[0]->get_value() / spin[1]->get_value();
ratio_zy = spin[2]->get_value() / spin[1]->get_value();
ratio_xz = spin[0]->get_value() / spin[2]->get_value();
ratio_yz = spin[1]->get_value() / spin[2]->get_value();
} else {
ratio_yx = 1.0;
ratio_zx = 1.0;
ratio_xy = 1.0;
ratio_zy = 1.0;
ratio_xz = 1.0;
ratio_yz = 1.0;
}
Copy link
Member

Choose a reason for hiding this comment

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

ratio_yx is the same as 1.0 / ratio_xy.
Maybe you could reduce the amount of member variables and computation needed by keeping only 3 and using * or / operations as appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could, I even tried it at first, but IIRC there were some precision issues.

@fire-forge
Copy link
Contributor

On a side note, I'm thinking about adding possibility to link 2 properties. This would be mostly useful for min/max values in ParticlesMaterial. So I wonder if the new property hint could be reused for that in the future.

If you do add this in the future, perhaps it could be used to link the corner radius and border width properties of StyleBoxFlat. I find that I usually use the same values for all 4 sides/corners and having the link button there would make it easier to edit them all at once.

image

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Should be good to merge once rebased.

Co-authored-by: redlamp <244062+redlamp@users.noreply.github.com>
@KoBeWi KoBeWi force-pushed the link_to_the_vector branch from 4799ee4 to 5553e27 Compare June 14, 2022 13:10
@akien-mga akien-mga merged commit 111a3ca into godotengine:master Jun 14, 2022
@akien-mga
Copy link
Member

Thanks!

@nathanfranke
Copy link
Contributor

On a side note, I'm thinking about adding possibility to link 2 properties. This would be mostly useful for min/max values in ParticlesMaterial. So I wonder if the new property hint could be reused for that in the future.

If you do add this in the future, perhaps it could be used to link the corner radius and border width properties of StyleBoxFlat. I find that I usually use the same values for all 4 sides/corners and having the link button there would make it easier to edit them all at once.

image

Maybe these should be a single property with type Vector4 with a new property hint like PROPERTY_HINT_SIDES that changes x/y/z/w to left/right/up/down (or whatever the best order is).

@YuriSizov
Copy link
Contributor

Vector4 wouldn't allow to explicitly indicate what each bit indicates, whether it's a side or a corner, without some inspector hackery. So I'm not sure. I was thinking about making a custom inspector widget here anyway, but so far only prototyped some stupid ideas and not much. But I think it would be the way to do it.

@timothyqiu
Copy link
Member

timothyqiu commented Dec 17, 2022

Cherry-picked for 3.6.

@timothyqiu timothyqiu removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 17, 2022
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.

Add a link/unlink option for subproperties like "scale"
8 participants