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

HYDRA-536 & HYDRA-537 : Background color and grid tests #112

Merged

Conversation

debloip-adsk
Copy link
Collaborator

No description provided.

@debloip-adsk debloip-adsk force-pushed the debloip/HYDRA-536/HYDRA-537/background-color-and-grid-tests branch from 7be617f to c32e27c Compare March 28, 2024 20:15
@debloip-adsk debloip-adsk marked this pull request as ready for review March 28, 2024 20:27
cmds.file(new=True, force=True)
mayaUtils.openNewScene()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the discussion on the color management PR, we talked about relying on the saved scene settings; however some settings are application-level and not saved per-scene, and some tests don't load a scene but instead construct it programmatically. So to remedy this, the suggestion of an optional parameter seems to me like the simplest compromise. I also added an analogous mayaUtils.openNewScene function, which opens a new scene and applies the test settings, and which is called before each test in replacement of cmds.file(new...) here. This, way the test settings are applied before each test and each new empty scene opened, but if we load an existing scene, we can let the scene's settings take over.

Note that in the spirit of favoring saved scene settings, we could consider removing the option of applying test settings in mayaUtils.openTestScene, and always relying on the saved scene settings, however for that we'd need to do a pass on our scene files to update them with the correct settings, which I feel would be better suited for a different PR (potentially alongside making MSAA disabled by default).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also with regards to @lilike-adsk's comment on providing a way to override certain default settings or using a skip flag, I think a simpler way is that if a test really needs specific settings, it can override setUp method and set whatever settings it needs, which means we don't have to implement a specific mechanism for this.

Comment on lines +34 to +46
cmds.displayRGBColor('background', 0.5, 0, 0)
self.compareBackgroundColor("background_red.jpg")
cmds.displayRGBColor('background', 0, 0.5, 0)
self.compareBackgroundColor("background_green.jpg")
cmds.displayRGBColor('background', 0, 0, 0.5)
self.compareBackgroundColor("background_blue.jpg")

cmds.displayRGBColor('background', 0, 0, 0)
self.compareBackgroundColor("background_black.jpg")
cmds.displayRGBColor('background', 0.5, 0.5, 0.5)
self.compareBackgroundColor("background_gray.jpg")
cmds.displayRGBColor('background', 1, 1, 1)
self.compareBackgroundColor("background_white.jpg")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems like we must use .jpg for playblasts to render the background color, using .png always rendered the background as transparent.

Comment on lines -29 to -30
cmds.file(new=1, f=1)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test hasn't been enabled in over a year; see the test CMakeLists. I updated it on principle in case we bring it back and based on how I think it should be updated, but I haven't actually run it.

Comment on lines -274 to +275
cmds.file(new=1, f=1)
mayaUtils.openNewScene()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test hasn't been enabled in over a year; see the test CMakeLists. I updated it on principle in case we bring it back and based on how I think it should be updated, but I haven't actually run it.

@@ -15,8 +15,6 @@
#
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed a bunch of settings changes that were happening under the hood and moved them to an optional step when opening scenes (except camera as that one was never used). I kept defaultRenderGlobals.imageFormat because this is set based on the extension of the file name being passed in.

@@ -29,6 +29,7 @@
pass

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed some unused methods, added openNewScene, and added applyTestSettings, which mostly applies the test settings that were previously in snapshot.

@debloip-adsk debloip-adsk self-assigned this Mar 28, 2024
@debloip-adsk debloip-adsk requested a review from ppt-adsk March 28, 2024 20:48
@debloip-adsk
Copy link
Collaborator Author

@debloip-adsk debloip-adsk added ready-for-merge Development process is finished, PR is ready for merge test labels Apr 2, 2024
@lilike-adsk lilike-adsk merged commit b412648 into dev Apr 2, 2024
10 of 11 checks passed
@lilike-adsk lilike-adsk deleted the debloip/HYDRA-536/HYDRA-537/background-color-and-grid-tests branch April 2, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants