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

Update project to Godot 4 beta 17 (animations still not fully updated) #13

Merged
merged 11 commits into from
Feb 6, 2023

Conversation

Brawmario
Copy link
Contributor

Godot commit that made this change necessary: godotengine/godot@bf15719

@akien-mga
Copy link
Contributor

I confirm that this change is needed for navigation code in 4.0 beta 16 and later.

There are more changes needed to be fully compatible with beta 16 though (shader and animation changes).

@Brawmario
Copy link
Contributor Author

I'll attempt to also fix these issues, I'll update this pull request with the new changes, thanks for pointing it out!

@Brawmario
Copy link
Contributor Author

I gave a shot at updating the project however I'm inexperienced with 3D animations and AnimationTrees. I fixed the shaders and the one call to the OneShot parameter to make the project work without any hard errors but some animations are still broken, like the player running cycle.

@Brawmario Brawmario changed the title Update use of NavigationAgent3D to match Godot 4 beta 16 API renames Update project Godot 4 beta 16 (animations still not fully updated) Jan 30, 2023
@geowarin
Copy link

geowarin commented Jan 30, 2023

I think all calls to travel() need to be wrapped in if state_machine.get_current_node() != "anim": like this:

func set_moving(value : bool):
	moving = value
	if moving:
		if state_machine.get_current_node() != "move":
			state_machine.travel("move")
	else:
		if state_machine.get_current_node() != "idle":
			state_machine.travel("idle")

or

func set_moving(value : bool):
	moving = value
	if moving:
		state_machine.travel("move", state_machine.get_current_node() != "move")
	else:
		state_machine.travel("idle", state_machine.get_current_node() != "idle")

I might be wrong though. Here is the PR that changed the AnimationStateMachine.

Edit: also see this issue

@Brawmario
Copy link
Contributor Author

Thanks @geowarin! That seems to be the case, I'll update this PR with the fix

@Brawmario
Copy link
Contributor Author

The only thing seemingly left is the Beetles T posing, only the death animation is functioning correctly and it doesn't seem to stem from the travel reseting the animation in anyway

@NathanLovato
Copy link
Contributor

Thank you very much for taking the time to help! Would you like a hand in figuring out the cause of this T-posing issue?

Renato Rotenberg added 2 commits January 31, 2023 10:00
@Brawmario
Copy link
Contributor Author

Thank you very much for taking the time to help! Would you like a hand in figuring out the cause of this T-posing issue?

Yes I would! I can't see why the animations for the beetle is broken when all the others aren't, I feel like this is the only outstanding issue right now stopping the project from fully being updated and functioning.

Also when opening the project in beta 16 it generates a bunch of PNG files extracted from the GLB files. I'll avoid committing them for now but I don't know how they should be dealt with.

@geowarin
Copy link

It might be this setting in the gltf files:

image

It changed recently

@Brawmario
Copy link
Contributor Author

I see. So apparently extracting the files is the way forward. I don't feel comfortable just committing the generated PNGs so I'll just ignore it for now, it doesn't seem to have anything with the broken Beetle animations.

@akien-mga
Copy link
Contributor

For the record, after godotengine/godot#72450 is merged, the animation travel changes may no longer be needed.

@Brawmario
Copy link
Contributor Author

Thanks for the heads up @akien-mga , I'll check and undo the travel changes when a next beta gets released with this update.

@geowarin
Copy link

geowarin commented Feb 1, 2023

I see. So apparently extracting the files is the way forward. I don't feel comfortable just committing the generated PNGs so I'll just ignore it for now, it doesn't seem to have anything with the broken Beetle animations.

Maybe not 😄 !

"Embed as uncompressed" will be an option again very soon:
godotengine/godot#72440

@akien-mga
Copy link
Contributor

akien-mga commented Feb 1, 2023

Yeah sorry for the back and forth on the API. As we get really close to RC and really freezing the API the maintainers find things which are absolutely needed to finalize and that requires a bit of (belated) experimentation on the API side :)

Beta 17 was just released, and we should have RC 1 next week which should hopefully stay compatible with Beta 17.

@lyuma
Copy link

lyuma commented Feb 1, 2023

@geowarin I advise against using embed as uncompressed for a demo project as it teaches people a bad workflow that will lead to huge wastings of 400% to 800% VRAM usage. (Unless this is a pixel art game)

If you need embedding, use Embed as Basis, or else consider adjusting your workflow so you can use gltf separate files with textures already extracted.

there's a reason Embed as Uncompressed was originally removed.

@lyuma
Copy link

lyuma commented Feb 1, 2023

Oh, did I mention embed as uncompressed has no mipmaps? 🤔
I wonder if we should have the gltf importer generate those at least

@geowarin
Copy link

geowarin commented Feb 1, 2023

Thanks for chiming in @lyuma. Having separate textures is usually a better choice, I think.

But this is a tradeoff (workflow is a bit more complex) that the guys at GDQuest will have to figure out. I think Godot maintainers did a great job with the naming to make this choice more obvious.

I'd hate to hijack this PR but can I get you all to look at this proposal of mine? I noticed that all the models in this project are facing backwards (towards +Z) and this was really confusing for me.

@Brawmario
Copy link
Contributor Author

Thanks for the insight @lyuma. However this PR is just primarily concerned in making the demo work again on the more recent Godot beta releases. I'll use the Embed as Uncompressed for now as it's the least intrusive option. Maybe after this PR is merged an issue could be opened to track this matter.

@NathanLovato
Copy link
Contributor

@lyuma what would you recommend regarding models? Reimporting them with textures and other files sitting next to the glb file? Or are we talking about something else?

@Brawmario Brawmario changed the title Update project Godot 4 beta 16 (animations still not fully updated) Update project to Godot 4 beta 17 (animations still not fully updated) Feb 1, 2023
Using "Embed as uncompressed" option for GLTF files for now
@NathanLovato
Copy link
Contributor

@Brawmario is this PR ready to review and merge? If so we'll review and merge this week.

@Brawmario
Copy link
Contributor Author

By me I'd say it's ready. I couldn't tackle fixing the beetles animation due to my lack of knowledge on 3d animation however the project now does run successfully on Beta 17 with all other API changes accounted for (as far as I could tell). I feel it would be best if this PR was reviewed and merged and have the remaining animation problem be tracked by a separate issue and tackled another time.

@NathanLovato NathanLovato merged commit 8353148 into gdquest-demos:main Feb 6, 2023
@NathanLovato
Copy link
Contributor

Thank you very much for the help!

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.

5 participants