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

Rename global transform notification to contain the word "global" #104352

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aaronfranke
Copy link
Member

I briefly mentioned this here: #87440 (comment)

Now the global transform notification has "global" or "GLOBAL" in its name, matching global_transform and such, mirroring the "local" versions. The old names are kept as deprecated aliases for compatibility.

@fire
Copy link
Member

fire commented Mar 19, 2025

This feels like an ugly api, but doesn’t seem to be a blocker (required pull request) for a godot engine game project.

@Mickeon
Copy link
Contributor

Mickeon commented Mar 19, 2025

It's a bit nasty because having it deprecated now may cause more confusion, due of the methods and notifications existing side-to-side with the originals.

Also CanvasItem has a similarly ambiguous naming scheme with its transform notification.

@lawnjelly
Copy link
Member

lawnjelly commented Mar 19, 2025

Agreed, this does seem to be compat breaking for modules etc?

Would have been better to have been done during that Godot 3 -> 4 phase when there were a lot of major renames.

Also, I'm not sure it's actually correct, especially for canvas items. Canvas items send their local transform to the server, not global.

EDIT: Actually I checked this morning and although local xforms are sent (in Node2D::set_transform()), canvas item xforms do seem to be propagated on the scene side, so I eat my words, I guess this notification could be said to be global.

It's still very debatable either way though I suspect. NOTIFICATION_TRANSFORM_CHANGED is vague, but as I understand it, although it signifies a change in global xform, it may or may not also accompany a change in local xform.

Related:
I often wonder why the NOTIFICATION system wasn't designed with at least a parameter, it would have been useful disambiguation in cases like this without having to e.g. send NOTIFICATION_GLOBAL_TRANSFORM_CHANGED and NOTIFICATION_LOCAL_TRANSFORM_CHANGED.

@KoBeWi
Copy link
Member

KoBeWi commented Mar 19, 2025

While the rename makes sense, there is another similar inconsistency: set_transform() and set_global_transform() methods. Should the former be renamed to set_local_transform()?

Ideally the methods and notifications should be consistent, but that would break compatibility, because the current name would have to change meaning.

Agreed, this does seem to be compat breaking for modules etc?

Yeah, it renames internal methods. Though the old names could be left as an alias, so module compatibility is preserved.

@fire
Copy link
Member

fire commented Mar 19, 2025

As mentioned earlier because of compatibility we can not remove the alternative renames until godot 5 and adding both has difficult usability

@Sauermann
Copy link
Contributor

As mentioned in godotengine/godot-proposals#3866, the word "global" is ambiguous.
I would prefer "canvas transform" or "viewport transform" depending on its meaning.

@aaronfranke aaronfranke force-pushed the notify-global-transform branch from 0576f30 to 34b29ca Compare March 20, 2025 00:36
@aaronfranke
Copy link
Member Author

@Sauermann The word global is consistent with 3D global transforms, referring to the world origin. I think "canvas transform" and "viewport transform" would be confusing, because this gives me the impression that it takes into account the Camera2D or scaling. But either way, this is established terminology in Godot with properties like global_transform.

This feels like an ugly api, but doesn’t seem to be a blocker (required pull request) for a godot engine game project.

Correct. It is not a blocker. It is a low priority enhancement.

Also CanvasItem has a similarly ambiguous naming scheme with its transform notification.

I'm not sure what you mean by that, this PR is already changing CanvasItem.

Agreed, this does seem to be compat breaking for modules etc?

I've updated the PR to include the old methods so that existing modules will keep working.

i.e. Canvas item notification should be NOTIFICATION_LOCAL_TRANSFORM_CHANGED

CanvasItem already has NOTIFICATION_LOCAL_TRANSFORM_CHANGED in master.

Should the former be renamed to set_local_transform()?

Maybe. I think a term by itself (like "transform") makes sense to refer to local automatically, but "local" can help clarify.

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