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

EMSUSD-872 - Adds the ability to System-lock a layer #3619

Merged

Conversation

AramAzhari-adsk
Copy link
Collaborator

@AramAzhari-adsk AramAzhari-adsk commented Feb 15, 2024

The system lock is a severity of locking a layer where it cannot be undone using UI. It is meant to be controlled through script or programmatically to prevent a user from editing or saving a layer.

The locking can be performed using the mayaUsdLayerEditor command with the lockLayer parameter:

  • lockLayer 0 is unlocked
  • lockLayer 1 is locked: Sets the PermissionToEdit to false
  • lockLayer 2 is system locked: Sets both the PermissionToEdit and PermissionToSave to false

Example of System Locking:

In the above example:

  • (1) asdf.usda is system locked, No Save, No Edit
  • As a result of the system lock, the anonymous layers (2), (3) cannot be saved since saving them will cause a change in asdf.usda. The anonymous layers can still be edited.
  • qwerty.usda changes can be saved since it doesn't affect the system locked parent.

Any Layer Editor operations that affect the locked parent are disabled on sublayers. This includes context menu operations.

@AramAzhari-adsk AramAzhari-adsk self-assigned this Feb 15, 2024
#define MAYAUSD_LAYERLOCKING_H

#include <mayaUsd/base/api.h>
#include <mayaUsd/nodes/proxyShapeBase.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.

The proxy shape will be updated on future iterations to add new functionality to layer locking.

}
} // namespace

void addSystemLockedLayer(const PXR_NS::SdfLayerRefPtr& layer)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are similar functions in layer muting. They will be extended to in the future iterations.

{
return (_isSharedLayer || (_layer && !_layer->PermissionToEdit()));
}
bool LayerTreeItem::isReadOnly() const { return (_isSharedLayer); }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: By design some of the functionalities should still be available if a layer has no permission to edit.

@@ -282,15 +281,21 @@ bool LayerTreeItem::isLocked() const { return _layer && _layer->PermissionToEdit

bool LayerTreeItem::appearsLocked() const
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Named similar to appearsMuted(). It has to do with the parent / children's state of lock.
By design, some of the sublayer's actions cannot be performed if the parent is locked.


bool LayerTreeItem::isSystemLocked() const { return MayaUsd::isLayerSystemLocked(_layer); }

bool LayerTreeItem::appearsSystemLocked() const
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Named similar to appearsLocked(). It has to do with the parent / children's state of system lock.
By design, some of the sublayer's actions cannot be performed if the parent is locked.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, so you want to be able to distinguish between something being locked and something being locked by its parent? So that is why appearsLocked does not check itself? OK, but the function name is a bit misleading. You can keep them like that, just a trap for the unwary :)

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 is similarly named to appearsMuted. The notion is that something "appears" locked but it's not really locked itself. Hence the word appear.

@@ -283,7 +289,11 @@ void LayerTreeItemDelegate::paint_ActionIcon(

QString tooltip;
if (_lastHitAction == action._name) {
tooltip = action._tooltip;
if (item->isSystemLocked()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When a layer is system locked, a different message is shown to the user to indicate that a layer cannot be unlocked or edited.

@@ -151,7 +151,7 @@ bool LayerTreeModel::canDropMimeData(
}

auto parentItem = layerItemFromIndex(parentIndex);
if (!parentItem || parentItem->isReadOnly()) {
if (!parentItem || parentItem->isReadOnly() || parentItem->isLocked()) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By design, immediate children of a locked parent layer cannot be reorganised (also covers the system lock layers by the shared constraint of PermissionToEdit = false).

@@ -235,7 +235,8 @@ bool LayerTreeModel::dropMimeData(

void LayerTreeModel::setEditTarget(LayerTreeItem* item)
{
if (!item->appearsMuted() && !item->isReadOnly()) {
if (!item->appearsMuted() && !item->isReadOnly() && !item->isLocked()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A locked or system locked layer cannot be targeted.

@@ -481,8 +482,11 @@ void LayerTreeModel::saveStage(QWidget* in_parent)
auto saveAllLayers = [this]() {
const auto layers = getAllNeedsSavingLayers();
for (auto layer : layers) {
if (!layer->isAnonymous())
layer->saveEditsNoPrompt();
if (!layer->isSystemLocked()) {
Copy link
Collaborator Author

@AramAzhari-adsk AramAzhari-adsk Feb 15, 2024

Choose a reason for hiding this comment

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

A System-locked layer cannot be saved by the extension that PermissionToSave = False. This also means that anonymous layers cannot be renamed until the layer is unlocked.

@@ -570,15 +574,15 @@ void LayerTreeModel::toggleMuteLayer(LayerTreeItem* item, bool* forcedState)

void LayerTreeModel::toggleLockLayer(LayerTreeItem* item, bool* forcedState /*= nullptr*/)
{
if (item->isInvalidLayer() || item->isSessionLayer())
if (item->isInvalidLayer() || item->isSessionLayer() || item->isSystemLocked())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No system locking is allowed through toggling.

for (const auto& dirtyLayer : StageLayersToSave._dirtyFileBackedLayers) {
_stageLayerMap.emplace(std::make_pair(dirtyLayer, stageName));
}

// We do not allow saving layers in any of the following conditions:
Copy link
Collaborator Author

@AramAzhari-adsk AramAzhari-adsk Feb 15, 2024

Choose a reason for hiding this comment

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

SaveLayerDialog will no longer list anonymous or file backed layers that are system locked. Same goes for anonymous layers whose parent layer is system locked.

@@ -805,11 +818,15 @@ class LockLayer : public BaseCmd
auto stage = getStage();
if (!stage)
return false;
if (_lockIt) {
if (_systemLock) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: For the time being, the undo state of a system lock goes into fully unlock. In a future design iterations a system-lock may get changed to never be undoable to prevent accidental unlocks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, it should return to the previous lock state (for example: locked -> system locked -> undo -> locked).

bool lockIt = true;
argParser.getFlagArgument(kLockLayerFlag, 0, lockIt);
int lockValue = 0;
// 0 = Unlocked
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wished there was an enum so that we would not have these magic values in the code.

@@ -12,6 +12,7 @@ target_sources(${PROJECT_NAME}
dynamicAttribute.cpp
editability.cpp
json.cpp
layerLocking.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not very important: indentation. If you make other adjustements in other files, you might as well fix it, otherwise it is really not critical.


LockedLayers& layers = getLockedLayers();
auto iter = layers.find(layer);
if (iter != layers.end()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that it is a set, I'm not sure why you bother checking if it is already in the set. Adding again would not do any harm.

@@ -80,7 +80,7 @@ class AbstractCommandHook
virtual void muteSubLayer(UsdLayer usdLayer, bool muteIt) = 0;

// lock or unlock the given layer
virtual void lockSubLayer(UsdLayer usdLayer, bool lockIt) = 0;
virtual void lockSubLayer(UsdLayer usdLayer, bool lockIt, bool systemLock) = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could pass 0, 1, 2 like the MEL command and even reuse the suggested enum to be used instead of 0, 1, 2.

@@ -138,6 +138,10 @@ class LayerTreeItem : public QStandardItem
bool isLocked() const;
// check if this layer is locked
bool appearsLocked() const;
// is the layer system locked?
bool isSystemLocked() const;
// check if this layer is system locked
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd change the comment to reflect that this check if parents are locked and not itself. I'd even add a similar comment in the cpp to help future readers understand that not checking itself is volontary and not an oversight.

pierrebai-adsk
pierrebai-adsk previously approved these changes Feb 15, 2024
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.

LGTM, only minor possible improvements.

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.

LGTM

@AramAzhari-adsk AramAzhari-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Feb 15, 2024
@samuelliu-adsk samuelliu-adsk added the workflows Related to in-context workflows label Feb 15, 2024
@samuelliu-adsk samuelliu-adsk merged commit cfee033 into dev Feb 15, 2024
11 checks passed
@seando-adsk seando-adsk deleted the azharia/EMSUSD-872/Adds-System-Lock-Funcitonality-To-Layers branch February 20, 2024 14:19
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 workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants