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

Procedural testing #1040

Merged
merged 9 commits into from
Jul 26, 2022
Merged

Procedural testing #1040

merged 9 commits into from
Jul 26, 2022

Conversation

AlvaroHG
Copy link
Collaborator

@AlvaroHG AlvaroHG commented Jul 14, 2022

New testing layer for Unity using Procedural houses.

  • Transform integer array to 3d polygon equivalent code and tested parity with procthor/houses package.
  • New HouseTemplate object for minimal house specification.
  • New test organization for future tests, under Procedural, divided by Rendering, Physics and Templates (layouts).
  • Testing suite for layouts.

Another PR incoming with Rendering tests.

Further improvements and cleanups of templates in the works.

@AlvaroHG AlvaroHG changed the base branch from main to nanna July 14, 2022 20:53
Copy link
Collaborator

@Lucaweihs Lucaweihs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick review, looks very nice :)! One thing I was expecting but didn't see was more pixel-to-pixel tests within ProcTHOR houses. Is this coming in a different PR?

unity/Assets/Scripts/ImageSynthesis/ImageSynthesis.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralData.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralData.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralData.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralTemplates.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralTools.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralTools.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralTools.cs Outdated Show resolved Hide resolved
unity/Assets/Scripts/ProceduralTools.cs.orig Outdated Show resolved Hide resolved
ai2thor/tests/test_unity.py Show resolved Hide resolved
@AlvaroHG AlvaroHG changed the title Proceudral testing Procedural testing Jul 25, 2022
@AlvaroHG
Copy link
Collaborator Author

Ready for merging, LGTM analysis seems to be hanging, but build passed.

@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 18 alerts and fixes 2 when merging 21b5450 into f44c798 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 8 for Useless assignment to local variable
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 2 for Use of default ToString()

1 similar comment
@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 18 alerts and fixes 2 when merging 21b5450 into f44c798 - view on LGTM.com

new alerts:

  • 9 for Use of default ToString()
  • 8 for Useless assignment to local variable
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 2 for Use of default ToString()

@Lucaweihs
Copy link
Collaborator

@AlvaroHG Can you open the LGTM link and double-check the 8 Useless assignment to local variable alerts? It would be good to remove these unneeded assignments and double check the logic makes sense without them.

After that, LGTM, please merge.

@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 13 alerts and fixes 2 when merging 593676d into 79e92c5 - view on LGTM.com

new alerts:

  • 7 for Use of default ToString()
  • 6 for Useless assignment to local variable

fixed alerts:

  • 2 for Use of default ToString()

@lgtm-com
Copy link

lgtm-com bot commented Jul 26, 2022

This pull request introduces 10 alerts and fixes 2 when merging c658efd into 79e92c5 - view on LGTM.com

new alerts:

  • 7 for Use of default ToString()
  • 3 for Useless assignment to local variable

fixed alerts:

  • 2 for Use of default ToString()

@AlvaroHG
Copy link
Collaborator Author

Merging now, remaining useless assignments are lambda variables that analyzer does not recognize

@AlvaroHG AlvaroHG merged commit 243aa90 into nanna Jul 26, 2022
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.

None yet

2 participants