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

Convert to Godot 4.1, remove binary data from scene files, prevent spurious diffs, and fix load warnings #30

Merged
merged 8 commits into from
Aug 19, 2023

Conversation

akien-mga
Copy link
Contributor

@akien-mga akien-mga commented Aug 17, 2023

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

  1. Port the project to Godot 4.1 (4.1.1-stable), handling an unfortunate compatibility breakage in the name of some nodes imported from .glb or .blend files. This means the demo won't run properly on Godot 4.0 anymore sadly.
  2. Remove a lot of binary data from some scenes which were very heavy due to complex meshes, collision shapes, or images being serialized in them as text. In many cases, this could be achieved through proper use of the Advanced Import Settings dialog for physics bodies, more on that below.
  3. Workaround issues where @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.
  4. Fix missing UIDs for ext_resources 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.

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:

588K    ./Main.tscn
13M     ./Box/DestroyedBox.tscn
5,3M    ./Player/GrenadeVisuals/grenade/grenade.tscn
956K    ./Environment/environment.tscn

This typically happens by mistake with the following pitfall:

  • Create geometry in Blender, import it in Godot as glb
  • Inherit that glb in a scene, make children editable, and modify the imported resource directly. Possibly hit "Make Unique" in the process
  • Boom, a ton of data is now saved in your .tscn instead of being read from the imported .glb

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 .glb

Before After
image image

The 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:

  • Enable "Physics"
  • Set "Body Type" to "Dynamic"
  • Configure "Shape Type" (here I used "Simple Convex", for others I kept the default "Decompose Convex" which can generate multiple shapes - I'm not an expert, feel free to review and change the shape types for the relevant .glb's)

image

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 and Main.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:

  • Coin, the @tool was just to preview the animation in the editor, unnecessary.
  • Environment, the script which scatters grass and generates a MultiMesh would run each time the scene is open, changing the generated MultiMesh. I just removed the keyword (so the MultiMesh stops changing in the editor, but it's still generated randomly when running the game).

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 @TokageItLab

Fix 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.

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.
@akien-mga akien-mga changed the title Convert to Godot 4.1, breaks compat with Godot 4.0 Convert to Godot 4.1, remove binary data from scene files, prevent spurious diffs, and fix load warnings Aug 17, 2023
@akien-mga akien-mga marked this pull request as ready for review August 17, 2023 21:49
@akien-mga
Copy link
Contributor Author

@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.

@NathanLovato
Copy link
Contributor

NathanLovato commented Aug 18, 2023

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.

@adamscott
Copy link

adamscott commented Aug 18, 2023

To do this, I just had to open Advanced Import Settings for DestroyedBox.glb, and for each "crate" mesh:

  • Enable "Physics"
  • Set "Body Type" to "Dynamic"
  • Configure "Shape Type" (here I used "Simple Convex", for others I kept the default "Decompose Convex" which can generate multiple shapes - I'm not an expert, feel free to review and change the shape types for the relevant .glb's)

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:

Capture d’écran 2023-08-18 090630

Here's on 4.1 (with this PR):

Capture d’écran 2023-08-18 090714

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)

@adamscott
Copy link

I'm investigating why the physics are all wonky in 4.1, if it's due to the PR or not.

@akien-mga
Copy link
Contributor Author

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.

@adamscott
Copy link

adamscott commented Aug 18, 2023

You're right, Rémi, it's not 4.1 per se. I followed your advice (checkout the first commit) and it's super stable.

image

@adamscott
Copy link

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?

@NathanLovato NathanLovato merged commit d6b7391 into gdquest-demos:main Aug 19, 2023
@adamscott
Copy link

Thanks @akien-mga !

@NathanLovato
Copy link
Contributor

NathanLovato commented Aug 19, 2023

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:

  1. Turned off rotation on the crates.
  2. Added a 100% rough physics material.
  3. In the level, moved the crates so they wouldn´t have this slight fall at the start, though alone, that didn´t completely solve the issue, they´d just fall a little slower.

That stabilized them on my end.

@akien-mga
Copy link
Contributor Author

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.

@Calinou
Copy link

Calinou commented Aug 21, 2023

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?

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.

@TokageItLab
Copy link

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.

Will be fixed in godotengine/godot#80813 if it will be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants