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

Smart signaling must emit MNodeMessages #3015

Merged
merged 10 commits into from
Apr 27, 2023

Conversation

JGamache-autodesk
Copy link
Collaborator

In order for clients to react to changes, the smart signalling should send either NodeDirty or NodeAttributeChanged to signal clients that they need to update.

In order for clients to react to changes, the smart signalling should
send either NodeDirty or NodeAttributeChanged to signal clients that
they need to update.

def nodeDirty(self, node, plug, listeners):
name = plug.partialName(False, False, False, False, False, True)
self.nodeDirtyReceived.setdefault(name, [0,])[0] += 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

That some bit-obscure Python trick. defaultdict would be clearer and avoid those [0] all over, of good old if/else :)

https://docs.python.org/3/library/collections.html#collections.defaultdict

pierrebai-adsk
pierrebai-adsk previously approved these changes Apr 6, 2023
Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

Code changes look okay, but I won't approve just yet since the preflight failed.

@JGamache-autodesk
Copy link
Collaborator Author

Preflight failure is valid. New code has undesired side-effect with caching. Will have to rework to expose these counters on a separate "listener" node that will connect on the proxy.

The update counters should not affect the stage, so they need to be on a
separate MPxNode.
Also added testing that changing the whole stage triggers a resync.
Anything that creates a MPlug on the proxy shape will end up causing a
leak that will help the USD stage survive the File->New/Open cycle. This
leak has been there for quite a while, so let's make sure we are not
re-using a tainted prim in the second test.

No need to even create a stage listener. The action of getting a plug on
the stage is sufficient to re-create that leak.
if (cacheIdNum != _lastKnownStageCacheId) {
const auto cacheId = UsdStageCache::Id::FromLongInt(cacheIdNum);
const auto stageCached
= cacheId.IsValid() && UsdUtilsStageCache::Get().Contains(cacheId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we have multiple stage cache in MayaUSD, for shared/unshared stages, and payload? See stageCache.h.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am aware of these caches now. But it looks like our clients prefer a cache accessible directly via USD APIs. This is also the cache used by the proxyShape for its outStageCacheId output, so I am not introducing anything new here.

# Helper to avoid copy-pasting the entire test
def doPosAndSizeTests(self, hasFunc, setFunc, getFunc, cmdFunc):
initialUpdateCount = cmds.getAttr('|transform1|proxyShape1.updateId')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note; a fix was already merged in dev. You might want to merge dev in your branch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I have merged with latest dev as I need the recent leak fix.

Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

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

I'm not sure about the stage caches and I think you will need to merge dev in to your branch, so I will wait for both of these before approving.

Copy link
Collaborator

@pierrebai-adsk pierrebai-adsk left a comment

Choose a reason for hiding this comment

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

Great!

@JGamache-autodesk JGamache-autodesk added the ready-for-merge Development process is finished, PR is ready for merge label Apr 27, 2023

#include <mutex>

using namespace MAYAUSD_NS_DEF;
Copy link
Collaborator

Choose a reason for hiding this comment

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

The macro is only for declaring the namespace so that it gets versioned.. When using the namespace, just use the name "MayaUsd" which will never change.

@seando-adsk seando-adsk merged commit d4b357c into dev Apr 27, 2023
@seando-adsk seando-adsk deleted the gamaj/smarter_signaling_for_renderers branch April 27, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proxy Related to base proxy shape ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants