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

Fast Configurable Shader Replacement #810

Merged
merged 5 commits into from
Jul 28, 2017
Merged

Fast Configurable Shader Replacement #810

merged 5 commits into from
Jul 28, 2017

Conversation

StephenHodgson
Copy link
Contributor

Fixes #628

  • Moved all obsolete shaders to an obsolete folder.
  • Changed all references from old shaders in materials to correctly use Fast Configurable.

@StephenHodgson
Copy link
Contributor Author

@dbastienMS let me know what you think

@dbastienMS
Copy link

dbastienMS commented Jul 27, 2017

Looks very good to me - I didn't look at the visual results for everything but I did a quick glance over which shader keywords were toggled on and it looks like things were mapped over well.

For the readme maybe make note of the fact that the old ones were moved to obsolete and that it is recommended that existing materials people may have created be moved over to fast configurable as it will improve performance for them.

Also just realized - should make note that due to the number of ifdefs there will be compile time implications similar to the standard shader if it is put in "always include". There's no real way around this as if Unity doesn't know which branches are being used, it must compile them all.

The editor script tries to do intelligent migration but I ran out of time to test it - it does handle some cases that the unity standard shader doesn't (coming from mobile shaders I believe).

Interested in how smooth you thought that process went, and how good you thought the editor script was in general.

@StephenHodgson
Copy link
Contributor Author

@dbastienMS don't forget to review and approve if you've signed off on this. Thanks.

Copy link

@dbastien dbastien left a comment

Choose a reason for hiding this comment

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

Ah, my bad - for some reason I had conflated write access with review rights in my mind.

  • Great work, thanks so much!

Copy link

@NeerajW NeerajW left a comment

Choose a reason for hiding this comment

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

It's worth documenting how to migrate from old shaders to the new one. I know people find this daunting and complex.

@dbastienMS
Copy link

@NeerajW it should handle migration as easily as moving from an existing Unity shader to standard - I believe I handle even more cases than the standard shader for migration.

It's definitely not perfect so if you have any examples of difficulties definitely send them along!

@NeerajW
Copy link

NeerajW commented Jul 28, 2017

So if I simply switch to this new shader everything should just work?

@dbastienMS
Copy link

That's the goal, yes. I ran out of time before the migration code was 100% complete, but it should handle a good deal of cases. Anything missing can be added to the migration part of the editor script.

@StephenHodgson StephenHodgson merged commit c956db0 into microsoft:master Jul 28, 2017
@StephenHodgson StephenHodgson deleted the HTK-ShaderUpdate branch July 28, 2017 21:29
@MatrixInception
Copy link

Hello, I tried to migrate from StandardFast to FastConfigurable, but it's not picking up the emission map correctly...

@dbastienMS
Copy link

Hi, thanks for taking the time to report!
Was this the only migration issue you had? Just checking in case I can knock a couple out at once.

@MatrixInception
Copy link

MatrixInception commented Aug 3, 2017

Thanks David. I like how the StandardFast shader had the same UI layout in Unity Editor as the Standard shader, so it was very easy to switch over a material from Standard to StandardFast in Unity. With the FastConfigurable shader, some of the sliders and object picker boxes are missing... for example "Metallic", "Smoothness" and "Height Map". Such missing features make migration more challenging. Need a guide to see what features are dropped and what features simply got a new name under a different button.

@dbastienMS
Copy link

StandardFast was a replacement for Unity's standard shader because Unity used to not have the concept of graphics tiers. It was just tripping some of the same defines that will happen now when you adjust tier settings.

Standard is what you'd call a PBR (physically based rendering) shader. FastConfigurable does not do PBR and therefore doesn't have some of the same concepts.

It's massively faster though, and you can always use the Standard shader (with appropriate quality settings) for small objects the user won't get close to.

The issue with Standard (and Standard fast) is primarily performance - in instances where usage of them occupies a moderate chunk of the field of view 60 fps may not be possible.

@dbastienMS
Copy link

When you say it isn't picking up the emission map correctly - do you mean it gives output different than you expect or that the texture isn't coming across at all?

I gave a quick test and an emission map did come across for me.

@MatrixInception
Copy link

Thanks for the explanation. Yes, when using Standard or StandardFast, the frame rate drops to 30 when the mesh occupies most of the field of view. As for the emission map, it gives an output different than expected. Under StandardFast, the emission with the same emission texture map was lighting up just a small part of the mesh object (as intended), but under FastConfigurable it's lighting up the entire mesh.

keveleigh pushed a commit to keveleigh/HoloToolkit-Unity that referenced this pull request Aug 11, 2017
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.

None yet

6 participants