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

Fix Control offset_* property types #98443

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

timoschwarzer
Copy link
Contributor

The documentation and the setter/getter methods state that the offset_* properties of Control nodes are supposed to be float, the property declarations use Variant::INT as property type though. This PR fixes that.

@timoschwarzer timoschwarzer requested a review from a team as a code owner October 23, 2024 00:30
@Chaosus
Copy link
Member

Chaosus commented Oct 23, 2024

If you change a core API, you've also needed to add a change to https://github.com/godotengine/godot/blob/master/misc/extension_api_validation/4.3-stable.expected.

@timoschwarzer
Copy link
Contributor Author

@Chaosus Thanks for the hint, updated!

@groud
Copy link
Member

groud commented Oct 24, 2024

The documentation is wrong, those are supposed to be INTs. This is because this value is in pixel and sprites/controls that are not aligned to the grid would could look blurry.

Changing this to float requires a proposal, but the documentation should be fixed for now.

@timoschwarzer
Copy link
Contributor Author

timoschwarzer commented Oct 24, 2024

@groud hmm, why is the internal type and the arguments to set_offset and get_offset real_t then and why does the project setting to snap controls to whole pixels exist in project settings then? 🤔 In any way, it's not just the documentation that is wrong here.

@groud
Copy link
Member

groud commented Oct 24, 2024

Ah my bad, you are right. I had wrong memory about this. I am still a bit worried about exposing them as float though, I feel it might be a bit more annoying to deal with in the editor. But well, I don't mind the change, we'll see if people complain.

@timoschwarzer
Copy link
Contributor Author

timoschwarzer commented Oct 24, 2024

@groud I could change the property hint to use 1px stepping. Then it shouldn't be annoying in the editor but you can set it to subpixel values in code.

I just noticed this issue after some time of going crazy while trying to set an offset to some float value in a script and the engine always converting it to a whole number, while insisting on it being a float. Turned out the docs were not telling the truth here 😄

My usecase here is that I render a pixel game in a viewport with supersampling, so an offset of 0.25 game pixels likely end up being multiple screen pixels.

@groud
Copy link
Member

groud commented Oct 24, 2024

@groud I could change the property hint to use 1px stepping. Then it shouldn't be annoying in the editor but you can set it to subpixel values in code.

Sounds good to me!

@timoschwarzer timoschwarzer force-pushed the fix/control-offset-type branch from 80c2972 to 14e5889 Compare October 24, 2024 21:09
@timoschwarzer
Copy link
Contributor Author

@groud Changed the hint to use 1px stepping as well as allowing to enter values less than -4096 and greater than 4096 since these limits seemed arbitrary/obsolete to me.

@timoschwarzer timoschwarzer force-pushed the fix/control-offset-type branch from 14e5889 to 08d8132 Compare November 2, 2024 00:00
@timoschwarzer timoschwarzer force-pushed the fix/control-offset-type branch from 08d8132 to 1daa9a1 Compare December 6, 2024 08:07
@akien-mga akien-mga changed the title Fix Control offset_* property types Fix Control offset_* property types Dec 6, 2024
@Repiteo Repiteo merged commit b91c38e into godotengine:master Dec 9, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 9, 2024

Thanks!

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