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

Adds tests for 45545 #21

Merged
merged 18 commits into from
Mar 13, 2021
Merged

Adds tests for 45545 #21

merged 18 commits into from
Mar 13, 2021

Conversation

abaire
Copy link
Contributor

@abaire abaire commented Feb 11, 2021

Adds automated tests for godotengine/godot#45545

@abaire
Copy link
Contributor Author

abaire commented Feb 11, 2021

Hey @fire I started writing some tests for the change to/removal of _sanitize_scene_name. Does this look like a reasonable start? Once I have a change that passes the basics I plan on adding tests to ensure bones are still properly attached as well.

@fire
Copy link
Member

fire commented Feb 12, 2021

I'll try to look near the weekend, don't wait for me.

@abaire abaire force-pushed the adds_tests_for_45545 branch from cf3ca19 to 8aa30b1 Compare February 13, 2021 16:39
@abaire abaire marked this pull request as ready for review February 14, 2021 17:39
@abaire
Copy link
Contributor Author

abaire commented Feb 14, 2021

Still a little bit of work to be done, but I think this is ready for a first pass.

Remaining bits:

  1. The emoji tests fail because the current JSON tokenizer can't parse them yet (@bruvzg proposed a fix in the review for Relaxes node name sanitization in gltf documents. godot#45545 that fixes this).
  2. The animation checking code doesn't work correctly yet, the resolution of clashing animation names produces results where the animation name doesn't match the node name since it's dependent on the ordering in the glTF file. I think we can probably just ignore the naming and simply check to make sure the animation properly references the correct node instead.
  3. Still need to add a skeleton test, but I expect it'll behave similarly to the animation case since I'm not planning on changing bone sanitization in 45545 (happy to do a followup though).

@abaire abaire changed the title [WIP] Adds tests for 45545 Adds tests for 45545 Feb 18, 2021
@abaire
Copy link
Contributor Author

abaire commented Feb 18, 2021

Animation checking code is fixed, added a skeleton test. Emoji tests still fail without the patch from @bruvzg (as expected), but generate nodes that are referenceable if you know that the emoji chars will be replaced by the replacement chars (I'd propose leaving as is if the improvement to the JSON tokenizer is likely to be accepted).

@fire
Copy link
Member

fire commented Mar 13, 2021

Is this ready to be merged?

@abaire
Copy link
Contributor Author

abaire commented Mar 13, 2021

I think so. It looks like I can't add reviewers on this repo though (and there doesn't seem to be an automatically assigned reviewer like on the main repo).

@fire fire merged commit 6f5e25d into godotengine:master Mar 13, 2021
@fire
Copy link
Member

fire commented Mar 13, 2021

Thanks for contributing to Godot Engine.

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.

3 participants