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

[al] make pushToPrim default to On #149

Merged
merged 1 commit into from
Jan 31, 2020

Conversation

pmolodo
Copy link
Contributor

@pmolodo pmolodo commented Dec 3, 2019

...a recent AL change (55e01cf) added
the ability to control the setting for pushToPrim for newly created
nodes via an optionVar, which is useful, but it also changed the default
from on to off.

In addition to being breaking behavior, this is also highly
non-intuitive for artists, who will be confused when they try to
transform the usd object, and it does nothing. I would also argue that
the ability to pushToPrim is the primary thing which distinguishes this
plugin from the Pixar plugin, and as such, is probably why most people
are using it in the first place.

NOTE: I haven't yet run the tests for this, as the tests are still not fully functional on the dev branch. Assuming we agree that this change should be made, we may wish to delay merging it in until the tests work, at which time we can ensure that this modification doesn't require making changes to any tests.

...a recent AL change (55e01cf) added
the ability to control the setting for pushToPrim for newly created
nodes via an optionVar, which is useful, but it also changed the default
from on to off.

In addition to being breaking behavior, this is also highly
non-intuitive for artists, who will be confused when they try to
transform the usd object, and it does nothing.  I would also argue that
the ability to pushToPrim is the primary thing which distinguishes this
plugin from the Pixar plugin, and as such, is probably why most people
are using it in the first place.
@kxl-adsk kxl-adsk requested a review from murphyeoin December 3, 2019 20:45
@cfmoore007
Copy link

@elrond79 - This also initially tripped us up ( and readAnimatedAttributes ).

We resorted to setting the default optionVar ourselves to work-around the issue.

If we're going for this one, why not the other?

@pmolodo
Copy link
Contributor Author

pmolodo commented Dec 3, 2019

Hmm, good point - I guess I wasn't aware the default for that had changed as well.

And yeah, for now we're also setting the default ourselves. Just think it might trip up newcomers.

@kxl-adsk
Copy link

Based on the conversation, it's unclear to me whether or not there is still something to do in this PR? To be precise, should readAnimatedAttributes have a matching default value?

Also, I see @robthebloke reviewed it. Do we need anyone else to approve it? (@murphyeoin)

@pmolodo
Copy link
Contributor Author

pmolodo commented Jan 20, 2020

Yeah, I guess it's a little unclear to me too. I guess I was hoping AL would chime in on the topic. @robthebloke or @murphyeoin - thoughts on also setting readAnimatedValues default to true? I guess the default for this changed slightly further back - in 97d3ff3.

(And for my vote, I don't think we need both Rob and Eoin to review... I just added them both hoping one would respond.)

pmolodo pushed a commit to LumaPictures/maya-usd that referenced this pull request Jan 22, 2020
…iles

added comment options when copying files
@pmolodo
Copy link
Contributor Author

pmolodo commented Jan 30, 2020

I decided to make readAnimatedValues it's own PR, since I could potentially see AL objecting to that, but not to this.

@pmolodo
Copy link
Contributor Author

pmolodo commented Jan 30, 2020

Oops, forgot to link it: #226

@kxl-adsk
Copy link

OK. Just double-checking - @murphyeoin you are good to make this change?

@kxl-adsk kxl-adsk merged commit 4af8bbe into Autodesk:dev Jan 31, 2020
@pmolodo
Copy link
Contributor Author

pmolodo commented Jan 31, 2020

Thanks!

robthebloke pushed a commit to AnimalLogic/maya-usd that referenced this pull request Sep 10, 2020
…-issue-VP2

PIPE-3073 - fix instance proxy issue vp2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants