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

Maya: Attributes are locked after publishing if they are locked in Camera Family #6073

Merged
merged 10 commits into from Jan 18, 2024

Conversation

moonyuet
Copy link
Member

@moonyuet moonyuet commented Dec 19, 2023

Changelog Description

This PR is to make sure unlock attributes only during the bake context, make sure attributes are relocked after to preserve the lock state of the original node being baked.

Additional info

It only happens when the camera is exported in raw maya scene(ma format).

Testing notes:

  1. Launch Maya via Launcher
  2. Create Camera Instance
  3. Freeze some Camera Attributes
  4. Publish
  5. Load the published camera by reference ma.
  6. The camera attributes should be locked if the camera attributes not being frozen before publishing

@ynbot
Copy link
Contributor

ynbot commented Dec 19, 2023

@ynbot ynbot added size/S Denotes a PR changes 100-499 lines, ignoring general files type: bug Something isn't working host: Maya labels Dec 19, 2023
@BigRoy
Copy link
Collaborator

BigRoy commented Dec 19, 2023

Could you elaborate on the use case for this? Why would you want to lock the attribute ON EXPORT instead of e.g. ON LOAD?

@moonyuet
Copy link
Member Author

moonyuet commented Dec 19, 2023

Could you elaborate on the use case for this? Why would you want to lock the attribute ON EXPORT instead of e.g. ON LOAD?

It's important for one of our users to lock the attributes all the time to prevent the accidental microscopic movements in viewport, especially when you have camera projections

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 19, 2023

It's important for one of our users to lock the attributes all the time to prevent the accidental microscopic movements in viewport, especially when you have camera projections

I'd argue it's better to ask WHY this is the case - because these can be easily locked on load (and should be locked actually!). As such, it looks like you're solving something in a drastic way for something that's easily solved with e.g. cmds.camera(lockTransform=True) on load?

It's really better to first phrase the problem, figure out the potential solutions before writing the code and then implement after. I mean it's fine if you want to spend your time writing code that's ok to be discarded to do it this way of course haha but we should be wary that just saying "because clients" is just the a way to say "we will do it this way" without having transparent what the issues are, what issues are being solved and whether or not there are better solutions out there.

So, from that "user" do you happen to have an explanation as to WHEN this issue occurs? Is it a lighting artist loading a camera? Is it a animation artist?

The solution you're proposing here likely works only maya -> maya whereas the on load solution can solve it for cameras, even if published from elsewhere.

@moonyuet moonyuet added sponsored Client endorsed or requested target: AYON labels Dec 19, 2023
@m-u-r-p-h-y
Copy link
Member

m-u-r-p-h-y commented Dec 19, 2023

Can't touch the code part, just to explain what we need:

  • we have hundreds of cameras in the scene used for photogrammetry projections.
  • cameras are locked, to prevent even micro-movements in the viewport
  • the issue is that during the publish the cameras are unlocked, exported but never locked back again (unnoticed) so the artist will end up with workfile where nobody is sure if the cameras were moved or not because they are unlocked after each publish

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 19, 2023

Can't touch the code part, just to explain what we need:

  • we have hundreds of cameras in the scene used for photogrammetry projections.

  • cameras are locked, to prevent even micro-movements in the viewport

  • the issue is that during the publish the cameras are unlocked, exported but never locked back again (unnoticed) so the artist will end up with workfile where nobody is sure if the cameras were moved or not because they are unlocked after each publish

Exactly what camera(lockTransform=True) intends to solve. Why not use that on load? The loading is managed via the pipeline, we have full control?

How is the user loading the data?

That they don't want to be accidentally moving the camera is a solid request of course, but it seems we're still avoiding the discussion on the actual UX of how it's caused and what options we have available to solve it.

This locking e.g. is also done by default when referencing a camera currently (and I believe also when loading a pointcache with cameras), see here:

def _lock_camera_transforms(self, nodes):
cameras = cmds.ls(nodes, type="camera")
if not cameras:
return
# Check the Maya version, lockTransform has been introduced since
# Maya 2016.5 Ext 2
version = int(cmds.about(version=True))
if version >= 2016:
for camera in cameras:
cmds.camera(camera, edit=True, lockTransform=True)
else:
self.log.warning("This version of Maya does not support locking of"
" transforms of cameras.")

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 19, 2023

  • the issue is that during the publish the cameras are unlocked

Do you mean that the user had originally LOCKED the attributes in their scene already with the intend to preserve that lock throughout the publish. Becuase if that is the case then that is definitely a thing to solve, as it'd mean some other areas of context managers aren't reverting what they should be.

@m-u-r-p-h-y
Copy link
Member

  • the issue is that during the publish the cameras are unlocked

Do you mean that the user had originally LOCKED the attributes in their scene already with the intend to preserve that lock throughout the publish. Becuase if that is the case then that is definitely a thing to solve, as it'd mean some other areas of context managers aren't reverting what they should be.

Exactly. It is not about loading or even preserving the state during publish, but not reverting the state of the scene (in this case cameras) to the original state before publishing, leaving it vulnerable to accidental changes.

When publishing is changing anything in the scene and we are not able to revert back peacefully we should force file reload to ensure data consistency.

@BigRoy
Copy link
Collaborator

BigRoy commented Dec 19, 2023

Exactly. It is not about loading or even preserving the state during publish, but not reverting the state of the scene (in this case cameras) to the original state before publishing, leaving it vulnerable to accidental changes.

Somehow I'm unable to comprehend the NOT + NOT here but I think you're saying:

  • Export the camera with the locked state as it was in the artist's scene before publish
  • After the publish the camera in artist's scene should still have the locked state as it was before the publish.

Agreed.

Anyway, the issue then is here: https://github.com/pypeclub/OpenPype/blob/ecf34f8f1a6bf4e5c9c8ff228b82f3d014792bce/openpype/hosts/maya/api/lib.py#L2793-L2803

This should be done with context manager instead so that anything baked, after baking has the same locked state as prior.

And voila, solved?

When publishing is changing anything in the scene and we are not able to revert back peacefully we should force file reload to ensure data consistency.

Haha - oh boy. We should definitely avoid that. If that's the case then we just need to fix more.

@moonyuet moonyuet force-pushed the bugfix/OP-6359_Maya-camera-state-beforeafter-publish branch from 0b626f9 to 55c3bdb Compare December 21, 2023 09:30
…are relocked after to preserve the lock state of the original node being baked
@moonyuet moonyuet changed the title Maya: options for locking channel attributes in extract camera maya scene Maya: Attributes are locked after publishing if they are locked in Camera Family Dec 22, 2023
@moonyuet moonyuet marked this pull request as ready for review December 22, 2023 14:18
Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

All my locked channels on Camera asset stayed intact and being locked as before the Publishing

image

Seems fine and working ok!

@moonyuet
Copy link
Member Author

moonyuet commented Jan 3, 2024

@antirotor can you check on the code changes? @LiborBatek would be great if you can check to see if the test is still worked as expected.
Thanks both!

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

Cant replicate the original issue of camera attributes getting unlocked after publish. Also guessing the testing notes have not been updated from the PR discussion.

Could we get the testing notes updated?

@moonyuet
Copy link
Member Author

Cant replicate the original issue of camera attributes getting unlocked after publish. Also guessing the testing notes have not been updated from the PR discussion.

Could we get the testing notes updated?

Please check the changelog again. Thank you!

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

The clickup ticket mentions only issues when publishing and nothing about loading.

I cant replicate the issue of unlocked attributes during publishing in latest develop.

@moonyuet would you mind testing in latest develop and update the testing notes?

@moonyuet
Copy link
Member Author

moonyuet commented Jan 15, 2024

The clickup ticket mentions only issues when publishing and nothing about loading.

I cant replicate the issue of unlocked attributes during publishing in latest develop.

@moonyuet would you mind testing in latest develop and update the testing notes?

It's the issue existed during context. All the camera attributes, which include the frozen camera attributes, are locked after publish. In the clickup, there is also a video(which is shared by Murphy), you can just take a look at it for better info. I also have updated testing notes a little bit for that, as I haven't worked for this ticket awhile(need some refresh). Use Reference Ma instead of reference abc during loading

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

Tested in Maya 2023. When dealing with ma reference loading, the locked attributes remain locked in latest develop. So I cant see any difference from develop to this PR?

Should this PR included a way of dealing with abc loading and locked attributes?
Could potentially add the locked attributes to the version metadata and locked them when loading.

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 17, 2024

The way I understood:

If the camera in the workfile has certain attributes locked, then those should also be locked upon loading. It's good to note that on load the transform attributes by default are already locked on load (no matter what) to avoid accidental changes by the artist, which they can toggle off via a button on the viewport in Maya. So this makes this PR more about attributes that are not the transforms, e.g. focal length. (But technically it's also about the transform attributes going into potentially other DCCs that allow reading locking state of attributes from the Alembic).

The issue was that before - that any keyed attribute (anything that 'needed baking', any attribute that was returned from _get_attrs) would end up unlocked in the published file. This should be solved with this PR.

The publishing has a variety of options:

  • Cameras can be baked to world-space, and they can just be baked as is (without that logic, which involves as far as I know only the .ma file. Still good to check both .ma and .abc with the option on and off)
    • When the option is off the full parent hierarchy I believe will be included in the .ma hierarchy.

In all of these cases:

  • The workfile should remain as is (no baked camera should be left dangling in the scene) nor should the locked state of the attributes have changed (they should still be locked or unlocked as they were prior to publishing)
  • The locked state in a new scene on loading both the .ma and the .abc should match the workfile - except that the transform attributes likely are 'always locked' on load automatically to avoid artists accidentally changing the camera.

I believe most of these issues only occurred originally if baking was enabled and the locked attributes had incoming connections (and thus needed 'baking' to an unlocked attribute). Looking at the original code I think that was indeed the issue and should be easily reproducable.

@LiborBatek
Copy link
Member

@tokejepsen @BigRoy the whole point of this PR being as simple as... if user locks some channel/attrib on the Camera it should stay in that state when loading it elsewhere using maya scene format. Nothing else... no Alembic ATM

Also did another test round in latest develop, and no, it doesn t work...if locking for ie transforms channels....it wont stay locked when loaded as maya scene aka Camera asset.

As simple as that guys.

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

Tested in Maya 2023. When updating the transform attributes (translate/rotate) get locked.

This might be a Maya bug rather than a pipeline bug.

Testing notes:

  1. Create camera and lock translate X.
  2. Publish camera and load into scene.
  3. Publish camera again and update in scene.

Initial load and translate X is locked correctly. On subsequent updating the translate/rotate channels gets locked.

@LiborBatek
Copy link
Member

failing in latest develop (using maya 2024) ...well, it didnt happened when pulling this PR...so whats the point here? I dont need any further evidence for that.

@BigRoy
Copy link
Collaborator

BigRoy commented Jan 17, 2024

Initial load and translate X is locked correctly. On subsequent updating the translate/rotate channels gets locked.

This would be a bug, even on first load these should be all locked. However, I'm unable to reproduce this myself in Maya 2024.

The transform (translate + rotate) should all be locked on load, due to the logic here.

@moonyuet
Copy link
Member Author

Initial load and translate X is locked correctly. On subsequent updating the translate/rotate channels gets locked.

This would be a bug, even on first load these should be all locked. However, I'm unable to reproduce this myself in Maya 2024.

The transform (translate + rotate) should all be locked on load, due to the logic here.

I tested this branch with Maya 2023.3, it still works the same with 2024. Please find the video for references.
https://github.com/ynput/OpenPype/assets/64118225/5e88c943-489a-4df4-b4e4-a66667e2fbb1

Copy link
Member

@tokejepsen tokejepsen left a comment

Choose a reason for hiding this comment

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

Looks good to me. Slight variation in testing in Maya 2023 but the end result is that the transforms and other attributes are locked.

@moonyuet moonyuet merged commit 05ba2d2 into develop Jan 18, 2024
@moonyuet moonyuet deleted the bugfix/OP-6359_Maya-camera-state-beforeafter-publish branch January 18, 2024 08:32
@ynbot ynbot added this to the next-patch milestone Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host: Maya size/S Denotes a PR changes 100-499 lines, ignoring general files sponsored Client endorsed or requested target: AYON target: OpenPype type: bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

7 participants