Nanna testing rendering #1044

AlvaroHG · 2022-07-23T01:48:37Z

Depends on #1040

Adds rendering unlit test in unity
Modifies Agent manager and Testing base to be able to get image payloads
Updates and fixes to Templates and TestTemplates
New python tests for RGB with light and depth.
- Found differences between rgb between fifo and wsgi servers for further investigation
- Two more rendering tests in the unity side for future work disabled due to inability to control resolution with UTF (unity test framework) or not trigger builds

Lucaweihs

Looks nice! I only did a quick review, please let me know if there are any pieces you'd like me to take a closer look at. I've added a few comments, mostly related to removing some dead code.

Lucaweihs · 2022-07-26T18:52:50Z

ai2thor/tests/test_unity.py

@@ -129,8 +140,38 @@ def assert_near(point1, point2, error_message=""):
        )


-def assert_images_near(image1, image2, max_mean_pixel_diff=1):
-    return np.mean(np.abs(image1 - image2).flatten()) <= max_mean_pixel_diff
+def assert_images_near(image1, image2, max_mean_pixel_diff=1, debug_save=False, filepath=""):


We're not actually asserting anything in this or the next function. Should we call them something like check_if_images_near or check_images_approx_equal?

Previous names, now changed

Lucaweihs · 2022-07-26T18:54:44Z

ai2thor/tests/test_unity.py

+    def depth_to_gray_rgb(data):
+        return (255.0 / data.max() * (data - data.min())).astype(np.uint8)
+
+    def save_image(name, image, flip_br=False):
+        import cv2
+        img = image
+        print("is none? {0} is none".format(img is None))
+        if flip_br:
+            img = cv2.cvtColor(image, cv2.COLOR_BGR2RGB)
+        cv2.imwrite("{}.png".format(name), img)


This looks like the same code as was added on line 84 above? Can we remove whichever is the duplicate?

Lucaweihs · 2022-07-26T18:56:31Z

ai2thor/tests/test_unity_procedural.py

+
+}
+
+# TODO rendering is different for fifo and wsgi server


Ooof, I forgot about that... We should probably deprecate the wsgi server sometime.

Lucaweihs · 2022-07-26T18:58:19Z

ai2thor/tests/test_unity_procedural.py

+    rgb_filename = "proc_rgb_lit.png"
+    ground_truth = cv2.imread(os.path.join(IMAGE_FOLDER_PATH, rgb_filename))


Works for now 👍 we should probably make it so that these test assets are downloaded from some S3 bucket in the future so that we don't grow the repository size in the future.

Yes that is why I created the save function to place in a directory and then use the ci_poll task to copy test images to s3. Next PR :)

Lucaweihs · 2022-07-26T19:00:34Z

tasks.py

+    import ai2thor.controller
+    import random
+    import json
+    import os
+    import cv2
+    import numpy as np
+    print(os.getcwd())
+    width = 300
+    height = 300
+    fov = 45
+    n = 20
+    import os
+    import json


I'm generally a fan of just having all imports at the top of the file, any desire not to do this? Same as the import of os on line 3821 above.

Yeah, I agree, for tasks which all vary widely in function this has been the standard each task imports it's things and runs in an independent context, removed duplicates though.

Lucaweihs · 2022-07-26T19:05:23Z

unity/Assets/Scripts/ProceduralTemplates.cs

+        // Creates problems with json serialization
+        // public Dictionary<string, WallRectangularHole> holes {
+        //     get { return 
+        //         doors
+        //             .Select(x => (x.Key, x.Value as WallRectangularHole))
+        //             .Concat(windows.Select(x => (x.Key, x.Value as WallRectangularHole))
+        //         ).ToDictionary(e => e.Key, e => e.Item2);
+        //     }
+        // }


Was not synced with current nanna merge, already removed

Lucaweihs · 2022-07-26T21:08:37Z

unity/Assets/Scripts/ProceduralTemplates.cs

+                    // if ( string.IsNullOrEmpty(obj.assetId) || !assetMap.ContainsKey(obj.assetId)) {
+                    //     return result;
+                    // }
+
+                    // var asset = assetMap.getAsset(obj.assetId);
+
+                    // var holeOffset = getHoleAssetBoundingBox(obj.assetId);


As above, removed

Lucaweihs · 2022-07-26T21:08:51Z

unity/Assets/Scripts/ProceduralTemplates.cs

+        // TODO: do global scaling on everything
+        // public static string scalePoints() {
+        //     (Math.Round(pair.Item1.row * scale, precision), Math.Round(pair.Item1.col * scale, precision)),
+        //         (Math.Round(pair.Item2.row * scale, precision), Math.Round(pair.Item2.col * scale, precision))
+        // }


Lucaweihs · 2022-07-26T21:09:23Z

unity/Assets/Scripts/ProceduralTools.cs

+        // public static ProceduralHouse houseFromRoom(
+
+    //     def find_walls(floorplan: np.array):
+    // walls = defaultdict(list)
+    // for row in range(len(floorplan) - 1):
+    //     for col in range(len(floorplan[0]) - 1):
+    //         a = floorplan[row, col]
+    //         b = floorplan[row, col + 1]
+    //         if a != b:
+    //             walls[(int(min(a, b)), int(max(a, b)))].append(
+    //                 ((row - 1, col), (row, col))
+    //             )
+    //         b = floorplan[row + 1, col]
+    //         if a != b:
+    //             walls[(int(min(a, b)), int(max(a, b)))].append(
+    //                 ((row, col - 1), (row, col))
+    //             )
+    // return walls
+


Lucaweihs · 2022-07-26T21:09:39Z

unity/Assets/Scripts/ProceduralTools.cs.orig

@@ -0,0 +1,2623 @@
+using System.Collections.Generic;


Is this .orig file needed?

Part of a merge, now gone

Lucaweihs · 2022-07-26T21:12:41Z

Any idea why the build is failing?

lgtm-com · 2022-07-27T21:41:01Z

This pull request introduces 7 alerts when merging ecce546 into 243aa90 - view on LGTM.com

new alerts:

3 for Module is imported more than once
1 for Unused local variable
1 for Container contents are never accessed
1 for Useless assignment to local variable
1 for Dereferenced variable may be null

…machine editor

lgtm-com · 2022-08-18T21:22:08Z

This pull request introduces 9 alerts when merging d63f276 into 7f79e43 - view on LGTM.com

new alerts:

4 for Module is imported more than once
2 for Useless assignment to local variable
1 for Module is imported with 'import' and 'import from'
1 for Container contents are never accessed
1 for Dereferenced variable may be null

AlvaroHG added 3 commits July 15, 2022 15:11

Testing debug

1a1e5ad

Tests

21b9fe6

Working python rendering tests

3b05547

AlvaroHG requested a review from Lucaweihs July 23, 2022 01:49

Merge

bedfc54

Lucaweihs reviewed Jul 26, 2022

View reviewed changes

AlvaroHG added 3 commits July 27, 2022 13:23

Merge branch 'nanna' into nanna-testing-rendering

44dcd19

TestRender

f81deb6

PR comments

ecce546

AlvaroHG added 19 commits July 27, 2022 17:09

Refactor

9b6a2ca

LGTM check

4195840

Removes Agent manager condition

30a3e90

Method capitalization

f5421ba

S3 routing of test output

ef324ac

Test directory variable

10ff6ea

fixes

0ed83b0

Fixes

8d57975

Adds to stdout

4fa63bb

Printing test for travis

accd396

Test data urls as json

c30bf9a

Figuring out multiprocess stdout for data urls

91838b8

old version of ci_merge_push_pytest_results seems to run

2afd44c

different flow

1f86035

s3 object fix

77fce9a

s3 bucket

0e81a75

import

4f653ea

Logger outs in ci

b769c40

epsilon change

c0a9239

AlvaroHG added 11 commits August 3, 2022 16:33

Logger more

00445e0

Not running ci_build

a975cb3

Fixes

7474c01

Bigger epsilon to make tests pass on build machine

81e0611

Save unity fail test

dd420f5

Fov on unity side

8aa6b87

Disabling unity test because lighting does not get disabled in build …

d40af9e

…machine editor

remove comments

c184656

Remove print

315f34d

Address last comments

7c7741c

Merge nanna

d63f276

AlvaroHG merged commit 9075a0f into nanna Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nanna testing rendering #1044

Nanna testing rendering #1044

AlvaroHG commented Jul 23, 2022 •

edited

Lucaweihs left a comment

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs Jul 26, 2022

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022 •

edited

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022 •

edited

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs Jul 26, 2022

AlvaroHG Jul 27, 2022

Lucaweihs commented Jul 26, 2022

lgtm-com bot commented Jul 27, 2022

lgtm-com bot commented Aug 18, 2022

		rgb_filename = "proc_rgb_lit.png"
		ground_truth = cv2.imread(os.path.join(IMAGE_FOLDER_PATH, rgb_filename))

Nanna testing rendering #1044

Nanna testing rendering #1044

Conversation

AlvaroHG commented Jul 23, 2022 • edited

Lucaweihs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlvaroHG Jul 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlvaroHG Jul 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucaweihs commented Jul 26, 2022

lgtm-com bot commented Jul 27, 2022

lgtm-com bot commented Aug 18, 2022

AlvaroHG commented Jul 23, 2022 •

edited

AlvaroHG Jul 27, 2022 •

edited

AlvaroHG Jul 27, 2022 •

edited