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

level output for the TextureBlock / switch for disabling level multiplication #10192

Merged
merged 1 commit into from
Apr 17, 2021

Conversation

rassie
Copy link
Contributor

@rassie rassie commented Apr 14, 2021

This PR is a playground for the discussion in https://forum.babylonjs.com/t/nme-make-texture-level-work-for-normal-maps-as-it-does-in-other-materials/19443/14. This code currently does not work (at all!), but might work someday.

@rassie rassie force-pushed the textureblocklevel branch 3 times, most recently from 4168d25 to 18b94bc Compare April 15, 2021 13:50
@rassie rassie changed the title [WIP, DONOTMERGE] level output for the TextureBlock level output for the TextureBlock / switch for disabling level multiplication Apr 15, 2021
@sebavan
Copy link
Member

sebavan commented Apr 15, 2021

LGTM aside the // FIXME block which needs to be removed. @Popov72 will provide a more in depth analysis :-)

@rassie
Copy link
Contributor Author

rassie commented Apr 15, 2021

LGTM aside the // FIXME block which needs to be removed.

Yeah, I couldn't find the reason for that. Would look into it again later today, but would also appreciate any help with that. Maybe there is something obvious that I've overlooked.

@rassie
Copy link
Contributor Author

rassie commented Apr 16, 2021

@sebavan @Popov72 have found and fixed the FIXME problem. This patch should be complete now.

@sebavan
Copy link
Member

sebavan commented Apr 16, 2021

@rassie, you have one lint issue:
image

And you should credit yourself in the dist/preview release/whats new.md file

So that the build will be all green and happy.

@Popov72 will do the thorough review soon.

@rassie
Copy link
Contributor Author

rassie commented Apr 16, 2021

Done!

Copy link
Contributor

@Popov72 Popov72 left a comment

Choose a reason for hiding this comment

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

LGTM: Great job!

One small thing is that 3 functions have been removed from bumpFragmentFunctions.fx and that could be a breaking change if someone was using them...

However, I don't think this code was supposed to be called by external users and having 3 x perturbNormal + 2 x cotangent_frame functions reduced to 1 x perturbNormal + 1 x cotangent_frame sounds good to me, so let's keep it!

src/Shaders/ShadersInclude/bumpFragment.fx Outdated Show resolved Hide resolved
@rassie
Copy link
Contributor Author

rassie commented Apr 16, 2021

LGTM: Great job!

One small thing is that 3 functions have been removed from bumpFragmentFunctions.fx and that could be a breaking change if someone was using them...

However, I don't think this code was supposed to be called by external users and having 3 x perturbNormal + 2 x cotangent_frame functions reduced to 1 x perturbNormal + 1 x cotangent_frame sounds good to me, so let's keep it!

It's a good thing there is a 5.0 release coming then ;) I'll fix the nits in a couple of hours when I'm not on mobile! Thanks!

@rassie
Copy link
Contributor Author

rassie commented Apr 16, 2021

Nits are done, thanks for the feedback!

@sebavan sebavan merged commit 4a17889 into BabylonJS:master Apr 17, 2021
@rassie rassie deleted the textureblocklevel branch April 17, 2021 17:03
rassie added a commit to rassie/Babylon.js that referenced this pull request Apr 23, 2021
Follow-up to 4a17889 (PR BabylonJS#10192). "Disable level multiplication" needs to be shown even if no texture is loaded, since otherwise developing a material from NME becomes more difficult.
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.

3 participants