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

[Dev→master] MRTK/Standard shader, update .mat, add test scene #1847

Merged
merged 7 commits into from Mar 22, 2018
Merged

[Dev→master] MRTK/Standard shader, update .mat, add test scene #1847

merged 7 commits into from Mar 22, 2018

Conversation

david-c-kline
Copy link

Port MixedRealityToolkit/Standard shader from Dev_Working_Branch, including related editor and runtime scripts.
Update materials that formerly used MixedRealityToolkit/FastConfigurable and Unity's Standard shader to use the MRTK shader.
Add StandardShader example scene from Dev_Working.

…RTK/Shader. Updated HolographicButton prefab with MRTK/Shader
@cre8ivepark
Copy link
Contributor

Updated InteractableObject and ObjectCollection example scene with MRTK/Standard shader.
2018-03-20 15_14_27-unity 2017 2 1p2 64bit - objectcollectionexample unity - johnnew - universal w
2018-03-20 15_13_40-unity 2017 2 1p2 64bit - interactableobjectexample unity - johnnew - universal

@david-c-kline
Copy link
Author

Thanks @cre8ivepark! Those look great!

@cre8ivepark cre8ivepark changed the title add standard shader from Dev_Working, update .mat, add test scene [Dev→master] MRTK/Standard shader, update .mat, add test scene Mar 20, 2018
@StephenHodgson
Copy link
Contributor

I thought we agreed we weren't putting new features into the master branch?

@timGerken
Copy link
Contributor

timGerken commented Mar 21, 2018

from the discussion around the @SimonDarksideJ YADFS document, summary from @davidkline-ms :

2. Create a new Dev branch for master to work on new features / fixes that our customers need sooner rather than later (SJ Agreed, but I thought that was Patch4 was about.)
a. Define t he key features (BoundingBox, Multi-Pointer) for our next full release out of master (SJ Agreed, but with some reservations)
b. Task a few people to cherry-pick from the current dev branch to bring over features that are done and work to complete those that are not (SJ Agreed)

@SimonDarksideJ
Copy link
Contributor

Those points are correct @timGerken the main outcome of that discussion surrounded the resources available to work in each area. @StephenHodgson and I are wholly focused on vNext at the moment, with support from @keveleigh where possible.
So if @davidkline-ms can spare the resource to patch small items in to master through a route agreed in the release strategy, that is also fine.
Although, I didn't think we had finalised that release strategy doc yet.

Anyway, this is a subject of discussion best done in slack rather than muddy a PR.

@StephenHodgson
Copy link
Contributor

Sounds good guys, just wanted to make sure we were on the same page.

@Cameron-Micka
Copy link
Member

Thank you @davidkline-ms and @cre8ivepark for porting this over! I will pull it down and take a look this afternoon.

@david-c-kline
Copy link
Author

@Cameron-Micka, I would be very interested to know if I missed updating or bringing over one or more pieces. It seems to work, but there is always a chance as this was a manual, not Git, cherry-pick.

Dave

@StephenHodgson
Copy link
Contributor

StephenHodgson commented Mar 21, 2018

Could this module be a standalone package without any other bits of the toolkit? Or do we have dependencies?

@cre8ivepark
Copy link
Contributor

@StephenHodgson HolographicButton prefab and some UX components are based on this shader.

@Cameron-Micka
Copy link
Member

Cameron-Micka commented Mar 21, 2018

@StephenHodgson This could certainly be a standalone package. Internally we have it in a NuGet package, and other packages (like our UI ones) include it as a dependency.

@david-c-kline
Copy link
Author

@StephenHodgson, this is one of the features that I personally get asked about often. While it is possible, as @Cameron-Micka mentions, to make this a separate package, a great deal of the value here is to get the improved shader and materials out to existing MRTK customers in our next patch release.

Copy link
Member

@Cameron-Micka Cameron-Micka left a comment

Choose a reason for hiding this comment

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

@davidkline-ms I just had a chance to pull down this PR and review it. It looks great sans a few minor things I fixed in a clone of your cpStdShader branch: https://github.com/Cameron-Micka/MixedRealityToolkit-Unity/tree/cpStdShader see commit 54b49d3, c090c2a, and 9432b7e. You should be able to pull this branch into yours and it will automatically fast forward you. (Unless there is another way to do this, I'm new to GitHub.)

On a side note, I could also update the shader to changes our team has made since my last PR if this is the right time and place to do that (it's 100% optional).

@StephenHodgson
Copy link
Contributor

StephenHodgson commented Mar 22, 2018

@Cameron-Micka so we're waiting for these changes to be merged?

If you have updates be sure to also open a PR against the Dev branch. I'm worried the two will start to drift too far away from each other again.

@Cameron-Micka
Copy link
Member

@StephenHodgson Yes, the above 3 commits should be merged in so this branch is on parity with what is currently in Dev_Working_Branch.

That's also a good point, if we want to take in updated changes to the shader I'll make sure to create a PR for Dev_Working_Branch.

@david-c-kline
Copy link
Author

@Cameron-Micka, I'll take your commits today.
Let's also update the shader. We might as well do one change to Patch4/master rather than two.

@Cameron-Micka
Copy link
Member

@davidkline-ms Perfect! I just update the shader in my branch, please see commit 8c9a942.

I will also create a PR for Dev_Working_Branch with these changes later today.

@david-c-kline
Copy link
Author

Pulled @Cameron-Micka's changes into my branch. Is there anything else left for master?

@Cameron-Micka
Copy link
Member

@davidkline-ms Looks perfect! I would approve the PR but it looks like I no longer can.

@david-c-kline
Copy link
Author

@StephenHodgson, @keveleigh, would you two care to approve on @Cameron-Micka's behalf?

Thanks!

@david-c-kline david-c-kline merged commit 66040d1 into microsoft:Patch4_Dev Mar 22, 2018
@david-c-kline david-c-kline deleted the cpStdShader branch March 23, 2018 21:41
@david-c-kline david-c-kline added this to To do in 2017.2.1.4 via automation Mar 26, 2018
@david-c-kline david-c-kline moved this from To do to Done in 2017.2.1.4 Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2017.2.1.4
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

7 participants