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

Loading mayaHydra should not set scene as modified. #216

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions lib/mayaHydra/mayaPlugin/plugin.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <maya/MFnPlugin.h>
#include <maya/MGlobal.h>
#include <maya/MSceneMessage.h>
#include <maya/MCommandResult.h>

#include <memory>
#include <vector>
Expand Down Expand Up @@ -78,6 +79,44 @@ namespace {
setenv(name.c_str(), value.c_str(), 1);
#endif
}

class SceneModifiedGuard
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Core change in the pull request. Create an RAII guard class to set the scene as unmodified after loading the mayaHydra plugin. Plugins often add default nodes on being loaded. This conceptually does not change the modified state of the scene.

{
public:

// Read modified state before
SceneModifiedGuard() : _wasModified(sceneModified())
{}

~SceneModifiedGuard()
{
// If modified flag became true, clear it.
if (sceneModified() && !_wasModified) {
MGlobal::executeCommand("file -modified 0");
}
}

private:

bool _wasModified{false};

// Scene modified query.
static bool sceneModified()
{
MCommandResult result;
auto status = MGlobal::executeCommand("file -query -modified", result);
if (status != MStatus::kSuccess) {
throw std::runtime_error("File modified query error in mayaHydra plugin load.");
}
int typedResult{0};
status = result.getResult(typedResult);
if (status != MStatus::kSuccess) {
throw std::runtime_error("Command result access error in mayaHydra plugin load.");
}
return (typedResult != 0);
}
};

}

void initialize()
Expand Down Expand Up @@ -132,6 +171,8 @@ void beforePluginUnloadCallback( const MStringArray& strs, void* clientData )

PLUGIN_EXPORT MStatus initializePlugin(MObject obj)
{
SceneModifiedGuard guard;

MString experimental("mayaHydra is experimental.");
MGlobal::displayWarning(experimental);

Expand Down
1 change: 1 addition & 0 deletions test/lib/mayaUsd/render/mayaToHydra/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ set(INTERACTIVE_TEST_SCRIPT_FILES
testCustomShadersNode.py
testMayaDefaultMaterial.py
testMayaLightingModes.py
testSceneModified.py
cpp/testColorPreferences.py
cpp/testCppFramework.py
cpp/testDataProducerExample.py
Expand Down
2 changes: 2 additions & 0 deletions test/lib/mayaUsd/render/mayaToHydra/testPolygonPrimitives.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ class TestPolygonPrimitives(mtohUtils.MayaHydraBaseTestCase):
IMAGE_DIFF_FAIL_THRESHOLD = 0.05
IMAGE_DIFF_FAIL_PERCENT = 1.5

_requiredPlugins = ['modelingToolkit']
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 plugin was being loaded for all tests, in fixturesUtils.py, but only this test was using it.


def compareSnapshot(self, referenceFilename, cameraDistance=15, imageVersion=None):
self.setBasicCam(cameraDistance)
cmds.refresh()
Expand Down
32 changes: 32 additions & 0 deletions test/lib/mayaUsd/render/mayaToHydra/testSceneModified.py
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Trivial test for modified scene.

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Copyright 2024 Autodesk
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
import maya.cmds as cmds
import fixturesUtils
import mtohUtils

class TestSceneModified(mtohUtils.MayaHydraBaseTestCase):
_file = __file__

# Base class setUp() defines HdStorm as the renderer.

def test_sceneModified(self):
# Though it modifies node 'defaultRenderGlobals' (as reported by
# 'ls -modified'), loading the mayaHydra plugin should not cause
# a scene modified warning, which causes spurious dialogs on file
# new or open to save what is essentially an empty file.
self.assertFalse(cmds.file(query=True, modified=True))

if __name__ == '__main__':
fixturesUtils.runTests(globals())
9 changes: 3 additions & 6 deletions test/testUtils/fixturesUtils.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,6 @@
import sys
import unittest

# Plugins that are bundled and loaded by default in a Maya installation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ArubaTesselator plugin is unused by our tests, and modelingToolkit was only used by a single test.

DEFAULT_PLUGINS = ['ArubaTessellator', 'modelingToolkit']

def _setUpClass(modulePathName, pluginName, initializeStandalone):
'''
Common code for setUpClass() and readOnlySetUpClass()
Expand All @@ -32,10 +29,10 @@ def _setUpClass(modulePathName, pluginName, initializeStandalone):

if pluginName:
import maya.cmds as cmds
wasModified = cmds.file(query=True, modified=True)
cmds.loadPlugin(pluginName, quiet=True)

for defaultPlugin in DEFAULT_PLUGINS:
cmds.loadPlugin(defaultPlugin, quiet=True)
isModified = cmds.file(query=True, modified=True)
assert isModified == wasModified, ('Loading plugin %s modified the scene' % pluginName)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assert that loading the test fixture plugin does not mark the scene as modified.


realPath = os.path.realpath(modulePathName)
return os.path.split(realPath)
Expand Down
57 changes: 2 additions & 55 deletions test/testUtils/mayaUtils.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,61 +43,6 @@
HD_STORM = "HdStormRendererPlugin"
HD_STORM_OVERRIDE = "mayaHydraRenderOverride_" + HD_STORM

def loadPlugin(pluginName):
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 dead code.

"""
Load all given plugins created or needed by maya-ufe-plugin
Args:
pluginName (str): The plugin name to load
Returns:
True if all plugins are loaded. False if a plugin failed to load
"""
try:
if not isPluginLoaded(pluginName):
cmds.loadPlugin( pluginName, quiet = True )
return True
except:
print(sys.exc_info()[1])
print("Unable to load %s" % pluginName)
return False

def isPluginLoaded(pluginName):
"""
Verifies that the given plugin is loaded
Args:
pluginName (str): The plugin name to verify
Returns:
True if the plugin is loaded. False if a plugin failed to load
"""
return cmds.pluginInfo( pluginName, loaded=True, query=True)

def isMayaUsdPluginLoaded():
"""
Load plugins needed by UFE tests.
Returns:
True if plugins loaded successfully. False if a plugin failed to load
"""
# Load the mayaUsdPlugin first.
if not loadPlugin("mayaUsdPlugin"):
return False

# Load the UFE support plugin, for ufeSelectCmd support. If this plugin
# isn't included in the distribution of Maya (e.g. Maya 2019 or 2020), use
# fallback test plugin.
if not (loadPlugin("ufeSupport") or loadPlugin("ufeTestCmdsPlugin")):
return False

# The renderSetup Python plugin registers a file new callback to Maya. On
# test application exit (in TbaseApp::cleanUp()), a file new is done and
# thus the file new callback is invoked. Unfortunately, this occurs after
# the Python interpreter has been finalized, which causes a crash. Since
# renderSetup is not needed for mayaUsd tests, unload it.
rs = 'renderSetup'
if cmds.pluginInfo(rs, q=True, loaded=True):
unloaded = cmds.unloadPlugin(rs)
return (unloaded[0] == rs)

return True

def createUfePathSegment(mayaPath):
"""
Create a UFE path from a given maya path and return the first segment.
Expand Down Expand Up @@ -152,6 +97,8 @@ def openNewScene(useTestSettings=True):
cmds.file(new=True, force=True)
if useTestSettings:
applyTestSettings()
# As this conceptually opens a new scene, set the file to unmodified.
cmds.file(modified=False)

def openTestScene(*args, useTestSettings=True):
filePath = testUtils.getTestScene(*args)
Expand Down
18 changes: 10 additions & 8 deletions test/testUtils/mtohUtils.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,6 @@
HD_STORM_OVERRIDE = "mayaHydraRenderOverride_" + HD_STORM
MAYAUSD_PLUGIN_NAME = 'mayaUsdPlugin'

def checkForPlugin(pluginName: str):
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 dead code.

try:
cmds.loadPlugin(pluginName)
except:
return False
return True

class MayaHydraBaseTestCase(unittest.TestCase, ImageDiffingTestCase):
'''Base class for mayaHydra unit tests.'''

Expand All @@ -60,7 +53,9 @@ class MayaHydraBaseTestCase(unittest.TestCase, ImageDiffingTestCase):
# Unloading mayaHydraFlowViewportAPILocator crashes Maya (HYDRA-1304).
# Unloading mtoa succeeds on Linux, but fails on Windows and macOS
# with "cannot be unloaded because it is still in use" error.
_pluginsCantUnload = ['mayaHydraFlowViewportAPILocator', 'mtoa']
# Unloading modelingToolkit fails with a
# "Dynamic unloading is not currently supported." error
_pluginsCantUnload = ['mayaHydraFlowViewportAPILocator', 'mtoa', 'modelingToolkit']

@classmethod
def setUpClass(cls):
Expand Down Expand Up @@ -106,8 +101,15 @@ def setUp(self):
# so open a new file before each test to minimize leftovers
# from previous tests.
mayaUtils.openNewScene()
modified = cmds.file(query=True, modified=True)
assert not modified, 'Internal test framework error: scene left as modified by mayaUtils.openNewScene()'

self.setHdStormRenderer()

# We've just opened a new scene, so we should not be modified. Setting
# Storm should conceptually not change that status.
cmds.file(modified=False)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be an assert? The comment makes it sound like this is checking that setting Storm does not change the modified state, but this just forces it to false

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 is setting it to unmodified, because the line above (setHdStormRenderer()) modifies the file. I can update the comment if you want.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What confuses me is the contradiction between :

[...] we should not be modified. Setting Storm should conceptually not change that status

and

(setHdStormRenderer()) modifies the file

It's unclear to me whether Storm should mark the file as modified. If it should, then why do force the flag to false, and if it shouldn't, then should this not be an assert, or otherwise point to a problem where setting Storm does change the modified flag without us wanting it to do so?


@classmethod
def tearDownClass(cls):
# Clean out the scene to allow all plugins to unload cleanly.
Expand Down