godotengine / godot-tests

Repository for Godot benchmarks, regression tests, etc.
MIT License
34 stars 33 forks source link

Adds tests for 45545 #21

Closed abaire closed 3 years ago

abaire commented 3 years ago

Adds automated tests for godotengine/godot#45545

abaire commented 3 years ago

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 commented 3 years ago

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

abaire commented 3 years ago

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 godotengine/godot/pull/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 commented 3 years ago

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 commented 3 years ago

Is this ready to be merged?

abaire commented 3 years ago

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 commented 3 years ago

Thanks for contributing to Godot Engine.