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.

Sign up for GitHub

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

Jump to bottom

Update refactoring sandbox #14

Closed
wants to merge 21 commits into from
Closed

Update refactoring sandbox #14

wants to merge 21 commits into from

Conversation

HamedSabri-adsk
Copy link
Contributor

  • Reference assembly cleanup ( Factored out scene assembly from base readJob class )
  • Run time path improvement for MacOSX and Linux
  • Fix variant switching after merge of proxy shapes
  • Fix import (Import works in ADSK and PXR plugins, AL translator now loads and imports)
  • Fix for building in debug
  • Added top-level cmake option "CMAKE_WANT_UFE_BUILD" with default value ON. When enabled, we (quietly) try to find UFE and if available we enable it and use it for building. If not available, then don't compile UFE features.
  • Linux translator plugins: Compilation and hdMaya camelCase fixes, mayaUsd_Translators and mayaUsd_Schemas Pixar PlugPlugin's load
  • Complete selection highlight support for VP2RenderDelegate
  • Fix build errors with Maya 2018
  • Added new flags to have better control over Cmake stages in the build script.

Huidong Chen and others added 21 commits July 28, 2019 19:37
MAYA-99646 minor build fix (#78)
cc0df71
Update README.md
5bd5612 
Fix typo in README!
@seando-adsk
MAYA-99741 - Build all 3 plugins build/work in sandbox (#77)
2717306 
* MAYA-99741 - Build all 3 plugins build/work in sandbox

* Override postConstructor in Maya & Pixar proxy shape and call
  the base class.
* Remove from the base class: PxrMayaHdImagingShape::GetOrCreateInstance();
* From all three plugins (Maya/Pixar/AL) call the base class
  and in Maya/Pixar (not AL) put the call to GetOrCreateInstance
  and conditionally call it using MayaUsdProxyShapePlugin::useVP2_NativeUSD_Rendering()

* MAYA-99741 - Build all 3 plugins build/work in sandbox

* Oops, fixed mistake in logic.
@seando-adsk
MAYA-99741 - Build all 3 plugins build/work in sandbox (#81)
a66591c 
* Add back the jinja.cmake file, which is only used when building
  inside the Maya build.
* Make sure the build and install folders have the variant.
MAYA-99646 Complete selection highlight support for VP2RenderDelegate (…
c034731 
…#83)

* MAYA-99646 Fix selection highlight issues

* Selection highlight for proxy selection from Outliner.
* Selection highlight for Xform prims from Outliner.
* Selection highlight for USD instancing objects.

* MAYA-99646 capitalize function name

* MAYA-99646 Differentiate the processing for "wasProxySelected" and "isProxySelected" case.
Make 3 main branches mergeable, and unify changes (#5) (#86)
aee93ad 
* Make sure the build and install folders have the variant (#2)
* Enable generating compile_commands.json by default
* Make sure the build and install folders have the variant 
* Fix typo in ReadMe
* temporarily revert line endings to windows to do merge
* standardize on linux-style line endings (newline only)

(cherry picked from commit 8073eaf)
@ppt-adsk
Linux translator plugins (#82)
1c70628 
* mayaUsd_Translators and mayaUsd_Schemas Pixar PlugPlugin's load.

* Removed translator plugin requirement for sidecar Maya plugin.

* Compilation and hdMaya camelCase fixes.

* Addressed code review feedback.
MAYA-99736 Fix build errors with Maya 2018 (#87)
0811aac 
Build with Maya 2018 has passed on Windows. Smoke test looks fine.
- Added support for stages in the build script. Comma-separated list …
b82530f 
…of stages can now be passed to the build system using --stages flag. By default 'clean','configure','build','install' stages are executed if this argument is not set. (#88)

(cherry picked from commit 2124591)
(cherry picked from commit e3524c668c50cb1cde80f4e9313229d8a9d27d16)
- Added the check for build stage that I missed in my initial commit.
3b79e1b 
(cherry picked from commit 8b3b348)
Revert "- Added the check for build stage that I missed in my initial…
578e24d 
… commit."

This reverts commit 3b79e1b.
@seando-adsk
MAYA-99741 - Build all 3 plugins build/work in sandbox (#90)
ea6fd60 
* Linux (gcc) and Mac (clang) fixes.
@seando-adsk
MAYA-99741 - Build all 3 plugins build/work in sandbox (#91)
1a92ea3 
* MAYA-99741 - Build all 3 plugins build/work in sandbox

* Added top-level cmake option "CMAKE_WANT_UFE_BUILD" with
  default value ON. When enabled, we (quietly) try to find UFE
  and if available we enable it and use it for building. If not
  available, then don't compile UFE features.
* Being an option means users UFE can be disabled if needed.
* Removed logic which looked for UFE only when devkit location
  was set.
* Replaced specialized "HDMAYA_UFE_BUILD" define with our generic
  "WANT_UFE_BUILD" define.
* Replaced xxx_INT vars with cmake configure logic.
* Added missing UFE to pxrUsd plugin.

* MAYA-99741 - Build all 3 plugins build/work in sandbox

* No need for extra #define in the promoted header file hdMaya.h.
  Just use the standard cmake add_definitions which works on all
  platforms and is consistent with the rest of our repo.
- trying to be explicit here for sake of clarity! (#89)
29ac3fb 
* - trying to be explicit here for sake of clarity!

* - Fix MacOSX python issue that was caused by overriding args. Thanks to Pierre!

(cherry picked from commit cc2ca87)
Merge pull request #92 from maya3d/sabrih/update_build_doc
a6a816d 
(cherry picked from commit 09d835f)
@seando-adsk
MAYA-99741 - Build all 3 plugins build/work in sandbox (#93)
79de90f 
* Fix for building in debug.
@ppt-adsk
Fix import (#97)
0760ad8 
* Fixed pxrUsd RPATH, and removed incorrect linking to mayaUsd_Schemas plugin.

* Import works in ADSK and PXR plugins.

* AL translator now loads and imports.

* Addressed review comments.
Fix variant switching after merge of proxy shapes (#98)
b1c679a
Run time path improvement for MacOSX and Linux(#99)
f0bf0f8 
* Brought over Pixar's runtime path routines into cmake/utils module and started using them in the project
* Fixed MacOSX build
* Added IS_WINDOWS, IS_LINUX, IS_MACOSX variables to determine the OS type
@ppt-adsk
Reference assembly cleanup (#100)
9f524d4 
* Factored out scene assembly from base readJob class.

* Factored code to remove REFACTOR_PROXY_SHAPE.

* Factored reference assembly out of instancerImager.

* Factored reference assembly out of instancerShapeAdapter.

* Enabled used of derived instancer shape adapter factory.
Merge remote-tracking branch 'origin/refactoring_sandbox' into update…
1210a88 
…_refactoring_sandbox
@HamedSabri-adsk HamedSabri-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Aug 12, 2019
@pmolodo
Copy link
Contributor

pmolodo commented Aug 12, 2019

So, there's obviously a fair number of duplicated commits (ie, 29ac3fb vs cc2ca87) introduced by this branch.

I'm guessing this is to make the external repo more similar to your internal repo, to make future pushes / merges / collaboration easier. If so, then I'm on board - anything to make repo management go smoother in the future is a good thing... and it's soon enough in the lifetime of the repo that there isn't too much duplication.

@kxl-adsk
Copy link

There are still some improvements to the process we can do, so please keep bringing them up @elrond79 & thx!

@pmolodo
Copy link
Contributor

pmolodo commented Aug 13, 2019

Do you have an internal repo with a different (and possibly non-git) history? And are you intending to keep it that way? If so, Pixar has been juggling the whole internal-vs-external thing fairly well for a while (at least from an outsider's perspective), where the internal repo has a different history than the external (at least I'm assuming, since they have so much private special sauce), so they may have some valuable tips / insight on how to manage this.
We at Luma don't have any specific experience with this workflow (all our internal repos are essentially the same as our external ones, minus some private downstream branches), but we do have a fair amount of experience with automating git repos in general, and we'd also be happy to help in whatever way we can.

@kxl-adsk
Copy link

Yes, we do have an internal repo, but there shouldn't be any difference with the public repo. We still have to figure out proper internal CI integration, but I hope we can avoid having to deal with two different repos.
Since we are on the subject or internal vs public, I was wondering about the public repo PR merging process, especially when changes need to propagate into downstream branches. Can you comment on it?

@pmolodo
Copy link
Contributor

pmolodo commented Aug 13, 2019

Yes, we do have an internal repo, but there shouldn't be any difference with the public repo.
If you don't have a different internal repo, how were these duplicate commits created? And do we still need them? No need to make the history much messier (and merges potentially much harder) if there's not a good reason for it.

As for PRs that need to be merged across, we have a few different options:

  1. Every time a PR for an upstream branch is accepted, an automated process kicks off a merge, a test, and then (ideally) pushes the merge back to github
  2. Same as above, except that you folks do it manually
  3. You mandate that the merging should be the responsibility of the folks making the PR, and that they should submit "pre-merged" PRs:
    3a. ...with one PR for each head
    3b. ...with a single PR at the most-downstream head, and clear notes on where any other upstream heads should be set to.
  4. You mandate that the merging should be the responsibility of the folks making the PR, and that once an upstream PR is accepted, they make new merge-commit PRs for downstream branches.

They all have drawbacks:

  1. Is the nicest, but obviously requires some CI setup. Also, you still have to have a policy for what happens when an auto-merge fails. If a merge fails, we need to pick a fallback of either 2 or 4.
  2. Obviously, the main downside is this means more work / maintenance for Autodesk folks. Plus, if it's a tricky merge conflict, the people submitting the PR will likely be better able to resolve it, due to more familiarity.
  3. I like this, except that as the PRs build up, there's no way for submitters to know what order they're be merged in, so the downstream merges will often not be fast-forwards. This will mean having to merge in a merge commit, creating ugly / unnecessary extra commits. Also may be a bit much to ask for a random contributor making a single-line commit to have to do a bunch of merges.
  4. Slows down the process significantly. Also, even well-meaning contributors may forget / not get around to doing this, meaning it turns into 2.

My vote is to aim for 1 (and fallback to 4 when auto-merges fail)... and in the meantime, we just do 2 or 4.

@kxl-adsk
Copy link

We are trying to understand the source of these duplicates. Will keep you posted.

@HamedSabri-adsk HamedSabri-adsk deleted the update_refactoring_sandbox branch August 15, 2019 14:39
kxl-adsk pushed a commit that referenced this pull request Aug 23, 2019
@sirpalee
Merge pull request #14 from superfunc/cmake
41ff741 
Make TBB a required dependency.
@dj-mcg dj-mcg mentioned this pull request Jun 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
do-not-merge-yet Development is not finished, PR not ready for merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HamedSabri-adsk @pmolodo @kxl-adsk @seando-adsk @ppt-adsk