Skip to content

Implement automatic scene conversion#191

Merged
mphe merged 6 commits intoSirRamEsq:masterfrom
mphe:shape_node_conversion
Apr 9, 2025
Merged

Implement automatic scene conversion#191
mphe merged 6 commits intoSirRamEsq:masterfrom
mphe:shape_node_conversion

Conversation

@mphe
Copy link
Collaborator

@mphe mphe commented Mar 22, 2025

This PR brings the foundation for running automatic version conversion on project startup.
Most importantly it implements scene conversion for the node type change to MeshInstance2D discussed in #172.
Also shortly before finishing this, Godot 4.4 was released, so I included the update to 4.4 as well, i.e. *.uid files and respective changes to the scenes.

The plugin checks if version information about SS2D exists in the project settings. If none was found or if it is different, all registered converter try to detect if changes are required.
If yes, a message will propup asking the user to confirm. The "OK" button is intentionally disabled for 3s to prevent users from confirming without reading.
When confirmed, all converters run sequentally and the success or failure is reported.
After success or when no conversion is required, the current version number is written to a custom internal project settings key to prevent having to run this procedure at every start.

That means the project conversion check will only run at project startup after initial install and after every version change until it was successful.
At the moment, the version number check is really just a != comparison. I think it should suffice in most cases and if a more sophisticated check is required, it can be added later.

In the 3-dot "Extra Options" menu there is also another entry to re-run the project conversion check manually. This might be useful for people copying scenes from older projects to newer projects.

To make testing easier, I have not yet included a commit with the MeshInstance2D conversion applied.
I will do so after the PR was approved.

mphe added 3 commits March 19, 2025 20:51
Only used at the moment for transitioning from Node2D to MeshInstance2D
but can be further expanded in the future if needed.
@mphe mphe requested a review from limbonaut March 22, 2025 23:05
Copy link
Collaborator

@limbonaut limbonaut left a comment

Choose a reason for hiding this comment

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

Looks good so far. Are you going to create a separate PR for the MeshInstance2D implementation?

var success := true

for i in converters.size():
if converters[i].convert():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we check if converter needs to convert the project once again? I assume not every converter needs to run on version upgrade.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I specified in the IVersionConverter class that if no conversion is necessary, convert() should do nothing.

# Perform conversion. Should return true and do nothing if no conversion is needed.
func convert() -> bool:

But for clarity and possible future changes, I can add another needs_conversion() check.

var analyzer := _get_test_analyzer()
assert_true(analyzer.contains_shapes())

analyzer.load("res://tests/unit/res://tests/unit/test_convert_tscn_node_types.gd")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Path broken?

assert_eq(analyzer._content_start_line, 11)

shape_ids.clear()
analyzer.load("res://tests/unit/res://tests/unit/test_convert_tscn_node_types.gd")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same

mphe added 2 commits March 30, 2025 17:22
- Fix broken paths
- Fix a bug where `TscnAnalyzer.load()` does not reset
  `_shape_script_ids`.
- Add another `needs_conversion()` check before running `convert()`.
  Technically not needed but makes it clearer.
@mphe
Copy link
Collaborator Author

mphe commented Mar 30, 2025

Fixed.
Fixing the path actually revealed another little bug where the state of TscnAnalyzer was not properly reset on load().
I also added the commit which includes the type change to MeshInstance2D.

Are you going to create a separate PR for the MeshInstance2D implementation?

Yes, this PR is only for the conversion process.

Edit: Also just pushed another minor bug fix unrelated to this PR where point changed signal handlers are not disconnected. Might be related to the screenshot in Discord.
Can also create a new PR for this if you want.

@mphe mphe marked this pull request as ready for review March 30, 2025 15:26
@limbonaut
Copy link
Collaborator

Can also create a new PR for this if you want.

I'm fine with it being here. Do you want to keep this open until MeshInstance PR?

@mphe
Copy link
Collaborator Author

mphe commented Apr 9, 2025

Nope, mesh instance PR takes a moment.
Ready to merge.

@mphe mphe merged commit 8707716 into SirRamEsq:master Apr 9, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants