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

implement per-stage normalscale #231

Merged
merged 4 commits into from
Dec 25, 2019
Merged

Conversation

illwieckz
Copy link
Member

@illwieckz illwieckz commented Dec 8, 2019

This PR sits above #230.
This is a demonstration of what ca be achieved thanks to #230.

Previously in #180 I introduced a per-material normalFormat option that would be able to flip channels. But then I discovered that other engines relies on a normalScale option that can both scale (and flip) channels.

A proper implementation required to be done on the stage scope.

Note that ioq3 seems to only read normalScale X Y while we are able to read normalScale X Y Z, if Z is missing, default one is used instead, to keep compatibility with material that may already exists.

As an example, we can look at some materials from tex-vega_src.dpkdir that have a flipped X channel.

Before:

bad normal map

After:

fixed normal map

They can be easily compared this way:

before/after fixed normal map

or with a slider there: https://imgsli.com/OTI4Mw

To do fix them, their materials were edited from this:

textures/shared_vega/panel01
{
	qer_editorImage textures/shared_vega_src/panel01_d

	diffuseMap  textures/shared_vega_src/panel01_d
	normalMap   textures/shared_vega_src/panel01_n
	specularMap textures/shared_vega_src/panel01_s
}

textures/shared_vega/trim03
{
	qer_editorImage textures/shared_vega_src/trim03_d

	diffuseMap  textures/shared_vega_src/trim03_d
	normalMap   textures/shared_vega_src/trim03_n
	specularmap textures/shared_vega_src/trim03_s
}

to this:

textures/shared_vega/panel01
{
	qer_editorImage textures/shared_vega_src/panel01_d

	{
		diffuseMap  textures/shared_vega_src/panel01_d
		normalMap   textures/shared_vega_src/panel01_n
		normalScale -1 1 1
		specularMap textures/shared_vega_src/panel01_s
	}
}

textures/shared_vega/trim03
{
	qer_editorImage textures/shared_vega_src/trim03_d

	{
		diffuseMap  textures/shared_vega_src/trim03_d
		normalMap   textures/shared_vega_src/trim03_n
		normalScale -1 1 1
		specularmap textures/shared_vega_src/trim03_s
	}
}

In the process we also moved from separate stages that has to be collapsed by the engine to a multi texture stage that is collapsed from the start.


Also note we inherited from XreaL another normalScale keyword that is currently only used by broken liquid shader, and this keyword is expecting an expression. We have to rename it. It is now named normalIntensity, this applies on XY normal map channels and can be computed on every frame by using expressions.

It would have been better to keep the normalScale keyword for that feature (as exists fresnelScale for example) and use normalChannelScale for the par-channel-scaling-and-flipping variable, but normalScale is already widely used by other idtech3 derivated engines, while our liquid shader is used by no one yet, even not by us.

@illwieckz
Copy link
Member Author

This seems ready to me.

@illwieckz illwieckz force-pushed the normalscale branch 2 times, most recently from a70f392 to bcae383 Compare December 13, 2019 10:21
@illwieckz illwieckz added T-Improvement Improvement for an existing feature A-Renderer labels Dec 13, 2019
@illwieckz illwieckz force-pushed the normalscale branch 2 times, most recently from 341255a to 42c5c43 Compare December 14, 2019 15:04
@illwieckz illwieckz force-pushed the normalscale branch 2 times, most recently from 7797e1f to 2e91742 Compare December 15, 2019 05:37
@illwieckz
Copy link
Member Author

illwieckz commented Dec 15, 2019

I added a commit to remove the r_normalScale glsl define. Since we pass u_NormalScale and compute it in all case, it may be better to feed u_NormalScale with r_normalScale value if exists.

@illwieckz
Copy link
Member Author

I noticed I forgot to make the dead liquid shader using u_normalScale, that's now fixed.
This is just to not let dead code rot until we revive it.

@illwieckz
Copy link
Member Author

illwieckz commented Dec 15, 2019

@slipher, I added a small commit I want your advice on.

This commit is just done to reduce boilerplate. But it replaces a definition by an affectation, do you think a compiler is able to optimize it, or, if not, this is negligible?

If you think it's OK I'll squash it, otherwise I'll drop it.

@slipher
Copy link
Member

slipher commented Dec 15, 2019

Removing r_normalScale define seems good. Not sure what you mean about stuff needing to be optimized as you simply removed the extra instructions from GLSL.

@illwieckz
Copy link
Member Author

illwieckz commented Dec 15, 2019

@slipher: Not sure what you mean about stuff needing to be optimized

instead of this in 8 places:

		vec3_t normalScale = { 1, 1, 1 };
		SetNormalScale( pStage, normalScale );

I do this in 8 places:

		vec3_t normalScale;
		SetNormalScale( pStage, normalScale );

And this in 1 place (in SetNormalScale):

 		normalScale[ 0 ] = 1;
		normalScale[ 1 ] = 1;
		normalScale[ 2 ] = 1;

The code is more compact this way, and less prone to errors, but in first case the vec3 is defined to 1,1,1, in the second case the vec3 is defined to 0,0,0 then set to 1,1,1.

@illwieckz illwieckz force-pushed the normalscale branch 2 times, most recently from 1a6b5c7 to ffd5a9e Compare December 19, 2019 01:53
@illwieckz
Copy link
Member Author

illwieckz commented Dec 24, 2019

Does anyone have something to say against that?

@illwieckz illwieckz merged commit a321153 into DaemonEngine:master Dec 25, 2019
@illwieckz illwieckz deleted the normalscale branch December 25, 2019 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Renderer T-Improvement Improvement for an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants