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

Deprecate Resource.setup_local_to_scene #67082

Conversation

Mickeon
Copy link
Contributor

@Mickeon Mickeon commented Oct 8, 2022

This PR has been updated to deprecate instead of removing. Compatibility is no longer broken because the method still works, but use is discouraged.

Continuation of #67080 and in a way, #67072.

To quote myself from Godot's RocketChat:

What? Why? I can't even find the reasoning as to why this happened, nor the reasoning as to why setup_local_to_scene is exposed to users in the first place! It was so difficult to explain in my documentation tweak. It does not feel right. Custom resources that would need this are expected to run their own specialised logic, their own functions, written by the more advanced user that knows what they're doing.

Above, I mentioned that it was a struggle to explain properly. And that is because it currently does something really trivial: emit the setup_local_to_scene_requested to potentially perform custom user logic. But internally, it is expected to be called only once during PackedScene.instantiate().

Because the user may also call it whenever they want, they need to understand the occasion the engine expects it to be called. Specifically, deserialization of a local_to_scene resource. But again, if the user is writing custom serialization for a Resource for whatever reason, they will write their own methods to do it, without ever calling setup_local_to_scene!

In conclusion, good riddance, please! ... See you in Godot 5...!

@Mickeon Mickeon requested review from a team as code owners October 8, 2022 13:17
@Chaosus Chaosus added this to the 4.x milestone Oct 8, 2022
@Mickeon Mickeon force-pushed the resource-screw-you-setup-local-to-scene branch 2 times, most recently from 9571884 to 4228937 Compare February 11, 2023 22:22
@Mickeon
Copy link
Contributor Author

Mickeon commented Feb 11, 2023

Rebased. Bumped.

@Mickeon Mickeon force-pushed the resource-screw-you-setup-local-to-scene branch from 4228937 to 8cc52d6 Compare February 11, 2023 22:31
@KoBeWi
Copy link
Member

KoBeWi commented Aug 15, 2023

Makes sense, but the method can't be removed, because it breaks compatibility. You can deprecate it and make no-op.

@akien-mga
Copy link
Member

akien-mga commented Aug 15, 2023

Makes sense, but the method can't be removed, because it breaks compatibility. You can deprecate it and make no-op.

As a reminder, deprecating means that the functionality still exists, but may be removed in the future.

If it's made no-op, there's no point in keeping it, as it breaks compatibility anyway (breaks projects that use it).

@Mickeon Mickeon force-pushed the resource-screw-you-setup-local-to-scene branch from 8cc52d6 to ea0f6e9 Compare September 6, 2023 19:12
@Mickeon Mickeon changed the title Unexpose Resource.setup_local_to_scene Deprecate Resource.setup_local_to_scene Sep 6, 2023
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 6, 2023

As edited above:

This PR has been updated to deprecate instead of removing. Compatibility is no longer broken because the method still works, but use is discouraged.

@Mickeon Mickeon force-pushed the resource-screw-you-setup-local-to-scene branch 3 times, most recently from 61d207d to 93bf474 Compare September 6, 2023 20:16
@Mickeon Mickeon force-pushed the resource-screw-you-setup-local-to-scene branch from 93bf474 to acd3851 Compare September 6, 2023 20:17
Good riddance.

Also modifies a note in ViewportTexture
@Mickeon Mickeon force-pushed the resource-screw-you-setup-local-to-scene branch from acd3851 to 0af2467 Compare September 6, 2023 21:38
@akien-mga akien-mga merged commit 6c42662 into godotengine:master Sep 8, 2023
@Mickeon
Copy link
Contributor Author

Mickeon commented Sep 8, 2023

Merged too soon? I suppose the description can be changed on the other PR.

@akien-mga
Copy link
Member

Yeah I forgot to remove it from my merge queue after commenting :)

@YuriSizov YuriSizov changed the title Deprecate Resource.setup_local_to_scene Deprecate Resource.setup_local_to_scene Sep 18, 2023
@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Nov 3, 2023
@Mickeon Mickeon deleted the resource-screw-you-setup-local-to-scene branch December 30, 2023 11:43
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.

7 participants