-
Notifications
You must be signed in to change notification settings - Fork 85
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
Convert to Godot 4.1, remove binary data from scene files, prevent spurious diffs, and fix load warnings #30
Conversation
Godot 4.1 sadly includes a compat breaking change in the way .glb and .blend files with import hints like `-convcol` are imported, when they contain a trailing `.001` number generated by Blender. Godot doesn't support `.` in node names, and in 4.0 it would simply remove the invalid characters. In 4.1+, it replaces invalid characters with `_`. This creates a clash between the names of objects inherited from a .glb scene with local modifications in the Godot scene, leading to a lot of errors. I solved it by replacing the names manually in the scene files to match what Godot 4.1 will generate. The `res://.godot` folder generated with Godot 4.0 should be removed to force a clean reimport with Godot 4.1.
Had to disable "Reset On Save" on the AnimationPlayer as it would keep changing the `grenade.material` file every time a new session is saved. There's a Godot bug to investigate there, as there should be no change to the material when just resetting the value which is already the default.
… with physics And split navmesh to a separate binary file.
…hysics And move navmesh to a separate .res file, amd remove duplicate terrain geometry.
These changes aim at minimizing the random property changes in scene files and resources due to editor state changes. The AnimationTree issue seems to be a glaring flaw, unlike AnimationPlayer with its RESET track, it has no way to ensure that base properties don't get modified by what's playing in the editor, and it even serializes playback subresources. Also remove unused and corrupted smoke_ball.tres.
@NathanLovato This is now ready for review, testing and merge. It's been a somewhat frustrating journey, lots of Godot issues and your team ran into many known pitfalls of the 3D asset pipeline... We still have a lot of work on the Godot side to make all this more reliable and user-friendly, but there are a number of takeaways here that you should be aware of for your demos and courses. |
Thanks much Remi. We were looking for an opportunity to fix the size problems and errors in this one, but the top priority is producing Godot 4 courses and staying afloat now, for the foreseeable future (we also have like 4-5 other demos like this one mostly done, needing an update, and pending release). I'll share with the team today for a test + a good read of your notes, and we'll merge once a handful of people tested this. |
I realized it was present on the Windows build, but I didn't check for this in the Linux build, but anyway the OS shouldn't impact this. Here's on 4.0:Here's on 4.1 (with this PR):It seems that the generated collision boxes make the box tower unstable. I suggest that we generate a simple collision box directly within the box model source file. A simple box like that shouldn't have multiple collision boxes. (edit: OOPS. nevermind, didn't realize that the multiple generated boxes are for the destroyed box parts, not the simple boxes) |
I'm investigating why the physics are all wonky in 4.1, if it's due to the PR or not. |
It's likely to be caused by changing the collision shapes indeed. You can checkout the first commit of this PR (simple conversion to 4.1, no cleanup of the files) to see if it behaves better. |
Bisected the wrong commit: 53e69a9 Unfortunately, I don't know why exactly. Looks like the boxes themselves are not the issue, but the generated physics underneath? |
Thanks @akien-mga ! |
Thanks again Rémi, both for taking time off your busy schedule to port this, but also to detail troubleshooting points. We appreciate that very much. I'm not 100% sure either why the physics engine struggled to stabilize the crates, I suppose it may be because the ground they're on has a generated convex collision shape and is not perfectly flat as a result? So I did 3 things:
That stabilized them on my end. |
Awesome! There's probably ways to get better collision shapes from the Advanced Scene Importer, there are various options available. I mostly kept the defaults as I'm not very experienced with what it does. |
Trimesh (concave) shapes should be preferred for static collision as they work more reliably in my experience. Convex collision is best reserved for rigid/character bodies when you can't use primitive collision shapes. |
Will be fixed in godotengine/godot#80813 if it will be merged. |
So this PR turned out pretty big, at first I aimed to do my changes in separate PRs to clean everything up, but this turned out to be impossible as there was too many issues closely related and fighting for diff space.
Summary
@tool
scripts and active AnimationTree would cause overwriting serialized properties every time a scene is saved, leading to a ton of Git noise and unnecessarily big and conflicting commits.heart_core_mat.tres
emission_energy_multiplier
changes value every time the main scene is saved #27.ext_resource
s by resaving all scenes and resources 3 times to make sure they all get a UID, and that it gets picked up by scenes which use them.res://Enemies/smoke_puff/smoke_ball.tres
sometimes fails parsing, and invalid UID warning spam #26.Detailed changes
Port to Godot 4.1 and handling .glb node name compatibility breakage
With godotengine/godot#75627, we sadly introduced a compatibility breakage in Godot 4.1. Before that PR, invalid characters in node names would be removed, but after that PR they are replaced by an underscore (
_
).This affects the import of files authored in Blender (.glb, .blend, .dae) when they use Blender's numbered de-duplication suffix (
Cube.001
,Cube.002
, etc.). The dot character (.
) is invalid in Godot node names so in Godot 4.1 it's now replaced by_
.This creates a clash between the names of objects inherited from a .glb scene with local modifications in the Godot scene, leading to a lot of errors. I solved it by replacing the names manually in the scene files to match what Godot 4.1 will generate.
IMPORTANT: The
res://.godot
folder generated with Godot 4.0 should be removed to force a clean reimport with Godot 4.1.This change means losing compatibility with Godot 4.0, as some of the meshes with editable children will break the same way as they would break from 4.0 -> 4.1 without my changes. That being said, the next series of commits may alleviate some of this issue.
Remove binary data from scene files, make better use of Advanced Import Settings dialog for physics bodies
This demo suffered from having a lot of binary data serialized as text in scene files. The most egregious cases:
This typically happens by mistake with the following pitfall:
I saw this pattern in the above files, where the meshes were imported from the .glb, but the physics bodies and collision shapes were added manually in the scene tree dock as child nodes to the imported meshes. This means that all the very complex generated collision shapes are saved in the scene as a PackedByteArray (typically more than 100,000 characters on a single line).
This can be avoided by making use of the Advanced Import Settings (double click on a .glb) to let it generate physics bodies and collision shapes on import. This generated data is then part of the imported file in
.godot/imported/
(and saved in binary format), and not part of your.tscn
file.With this I could e.g. redo
DestroyedBox.tscn
, which was 100% decoupled from its .glb and had embedded all mesh and collision data, to a version 100% coupled to its .glbThe yellow coloring means that those nodes are inherited from the .glb, so only their modifications are saved in the scene - if you modify their mesh by making it unique, it will be saved in the .tscn.
To do this, I just had to open Advanced Import Settings for
DestroyedBox.glb
, and for each "crate" mesh:Likewise, baking a NavigationMesh writes it in the scene by default, which sucks. So you should make sure to save it as a separate
.res
resource (not.tres
, otherwise it's going to be huge again due to the way binary data is serialized to text).grenade.tscn
had another pitfall, where the Mesh data inherited from the .glb had been modified to be able to assign a material. That's not the recommended way to add materials to imported meshes! You should instead use the "Surface Material Override" property on the MeshInstance.I did that, and saved the material as a separate
.material
(binary, equivalent to.res
) because it contained a lot of binary data (ImageTextures). Those could be saved back to PNGs if you want to keep a material in.tres
format.As a side note, I fixed both
environment.tscn
andMain.tscn
, which seem to be too competing implementations for the level,environment.tscn
being unused. Might need a cleanup.Important: Please review that everything still runs as intended in
Main.tscn
. There was notably a duplicate version of the level mesh and collision shape, which I removed, and it still seems to work fine using only the mesh and generated collision shape from the .glb.Workaround unwanted diffs due to changing state in the editor
Godot has the great feature to let you run scripts and animations in the editor, but this comes with the common pitfall of saving non deterministic property values to your scenes and resources.
I had to remove the
@tool
keyword from the two files which used it to reduce these unwanted changes each time their scenes were saved:@tool
was just to preview the animation in the editor, unnecessary.Likewise, and possibly worse, the scenes using AnimationTree kept having a bazillion values for positions and rotations changing. That's because AnimationTrees were
active
and autoplaying idle animations, and thus when you save the scene, it would save whatever is the current state.I disabled them by default, and added code to enable them in
_ready
.AnimationPlayer has a
RESET
track system and "Reset On Save" option to make sure that the default state is kept consistent, but AnimationTree doesn't seem to have a system like this. It's very easy to mess up. FYI @lyuma @TokageItLabFix missing UIDs by resaving everything
That's a weirdness I haven't fully understood yet, Godot sometimes fails to assign a UID to a newly created scene or resource, but re-saving it later adds it. And sometimes it just decides that this scene or resource should have a different UID... It's pretty messy. I think we've fixed a number of issues with this in Godot 4.1, so hopefully now it's going to be more stable.
Testing welcome on different machines / OSes to make sure things stay consistent.