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

Make enum/constant binds 64-bit. #61991

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jun 13, 2022

  • Changes constant / enum data type to be 64-bit int.

@KoBeWi
Copy link
Member

KoBeWi commented Jun 13, 2022

I assume the 64bit change is due to running out of space for constants?
There are a few unused constants, so this might be not necessary (see #60458), but could be useful in the future.

@bruvzg
Copy link
Member Author

bruvzg commented Jun 13, 2022

I assume the 64bit change is due to running out of space for constants?

Yes, but it's probably good it bump it to 64-bit anyway, since the rest of the integers are 64-bit.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jun 14, 2022

I see why this is needed, and this kind of visibility is needed for more than just fonts on labels. Many controls, containers especially, have very important properties hidden in overrides. But I'm not sure if this is a good way to do it.

First of all, it's not clear to the user that it's a duplicated property. So they may get confused why there are two ways to set something very similar. The only clue would be that the tooltip should show it as theme_override_fonts/font instead of just font, but that's easy to miss. And the UI for overriding itself is kind of unexpected here. Why other same level properties can just be set and unset to default, but this one has a checkbox next to it? Why do other properties have fallback values, and this one must be set to empty and you don't know what it would fallback to?

Granted, these are existing problems with overrides as well, but as long as they are treated as a special case, it's less severe. Here they would be mixed with the normal properties, and those usability nuances get front and center.

Second, this makes it less obvious for some properties that they are controlled by themes. I specifically renamed the Custom ... sections to Theme Override/... sections to make sure that users understand that there is a relation. But this goes the opposite way. IMO, this incentivizes to avoid themes and set everything ad-hoc, on per-control basis, and I'd argue we need to incentivize using themes more, unless the opposite is actually needed. Especially now that we have a way to define type variations and thus make themes even more reusable.


So overall, I'm not sure. The way theme properties are presented in the inspector does not respect the inheritance chain, and doesn't respect the importance of those properties for each control. I'm kind of of a mind that there should be another "theme/configuration" tab, or something. But then again, reduz wants to move signals and groups into the inspector instead of having individual tabs, so this kind of solution wouldn't be welcome. I don't have a better idea at the moment.

@bruvzg bruvzg changed the title Add support for property shortcuts, to display the same property multiple times. Make enum/constant binds 64-bit. Jun 17, 2022
@bruvzg bruvzg force-pushed the property_shortcut branch from ec6a42b to 860e246 Compare June 17, 2022 13:36
@bruvzg
Copy link
Member Author

bruvzg commented Jun 17, 2022

Superseded by #62139, but make enums / constants 64-bit might be still useful, so keeping it for this purpose only.

@akien-mga akien-mga merged commit dd93ae6 into godotengine:master Jun 17, 2022
@akien-mga
Copy link
Member

Thanks!

@bruvzg bruvzg deleted the property_shortcut branch June 17, 2022 21:03
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.

6 participants