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 UID support in MultiplayerSpawner #99712

Merged

Conversation

KoBeWi
Copy link
Member

@KoBeWi KoBeWi commented Nov 26, 2024

Fixes #99706

The bug occurred because setting a path will use _set() on path index, instead of _set_spawnable_scenes(). idk how I missed it previously.

The main problem is that MultiplayerSpawner must operate on paths, but the editor wants it to store the paths as UID. I removed the uid field added in #99137. Now all paths are stored as paths internally, but saved as UID, with the conversion happening inside setters/getters. This might affect scene loading, but shouldn't impact performance of the spawner's operation.

@KoBeWi KoBeWi added this to the 4.4 milestone Nov 26, 2024
@KoBeWi KoBeWi requested a review from a team as a code owner November 26, 2024 15:26
@Repiteo Repiteo requested a review from Faless November 26, 2024 16:24
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Pressing the "Add Element" button in the inspector now spams:

ERROR: Condition "!unique_ids.has(p_id)" is true. Returning: String()
   at: get_id_path (core/io/resource_uid.cpp:133)

@KoBeWi KoBeWi force-pushed the bug_spawner_spawns_new_bugs_to_fix branch from ffba7c3 to 4fdb8ad Compare November 27, 2024 14:07
@KoBeWi KoBeWi requested a review from a team as a code owner November 27, 2024 14:07
@KoBeWi KoBeWi force-pushed the bug_spawner_spawns_new_bugs_to_fix branch from 4fdb8ad to 67b95f3 Compare December 3, 2024 21:11
@akien-mga akien-mga requested a review from Faless December 3, 2024 22:15
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.

Core changes look good to me.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Repiteo Repiteo merged commit ab58a33 into godotengine:master Dec 4, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 4, 2024

Thanks!

@KoBeWi KoBeWi deleted the bug_spawner_spawns_new_bugs_to_fix branch December 4, 2024 18:23
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.

Godot v4.4.dev5 breaks MultiplayerSpawner: Condition "!ResourceLoader::exists(p_path)" is true.
4 participants