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

use only one texCoords per surface #194

Merged
merged 1 commit into from Mar 31, 2019

Conversation

Projects
None yet
2 participants
@illwieckz
Copy link
Member

illwieckz commented Mar 30, 2019

The extra compute and boilerplate code seem useless and it already ended in bugs when parallax offset was applied to diffuse, normal, specular but nor glow map, and I would find stupid to add more boilerplate when height map will be implemented.

Diffuse/normal/specular/glow/height maps are component of the same material, so there is no reason to handle their coordinates separately.

Doing this would also avoid the need for a lot of ifdef code when normal/specular/glow/parallax support is disabled.

And well, that extra code seems useless even when they are enabled.

It unify the symbol names to var_TexCoords.

Alongside the var_TexDiffuse, var_TexNormal, var_TexSpecular, var_TexGlow there was some occurrence of var_Tex.

Some comments in already rewritten code told that Tr3b had issue with some driver/hardware suffered from "too much var" issue when passed from vp to fp shaders, leading him to abuse some variables,
see 72268b, even if I have not faced this issue with hardware that is so old the game is unplayable, this is an even better way to reduce the number of variables passed from vp to fp.

For multimap material (diffuse/normal/specular/glow), the texCoords used is the diffuse one.

Note that we already computed the parallax texOffset from diffuse coords only, and applied it to other maps as it would be stupid and inefficient to compute the parallax texOffset on each normal/specular/glow coords.

@illwieckz illwieckz force-pushed the illwieckz:texcoords branch 2 times, most recently from 7b4dba4 to 4079b72 Mar 30, 2019

@illwieckz

This comment has been minimized.

Copy link
Member Author

illwieckz commented Mar 31, 2019

So about the idea itself @cmf028 said this is the way to go, I would appreciate someone validating the implementation too.

Fuma: oooo, the seperate texcoord transforms are being removed!
Fuma: nice
Fuma: This would also improve wavefront occupency on AMD GCN
illwieckz: Fuma, so you confirm it's the thing to do?
illwieckz: what do you mean by “wavefront occupency”? what's specific to GCN on that part?
Fuma: you are aware that a fragment shader runs once per pixel (simplification, but yeah)
Fuma: each of those fragment shader invocations is a "wave"
Fuma: multiple waves can run in parallel in a single compute unit
Fuma: however, the number of waves than can be run in parallel is limited by various things
Fuma: increasing "wavefront occupency" =>: increasing the number of waves that can be run in parallel
perturbed: how does the change allow more waves to run?
Fuma: for GCN, having more than 16 floats interpolating in the fragment shader imposes extra restrictions
Fuma: ...that limit the number of waves that can be run in parallel
Fuma: Source: http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/05/GCNPerformanceTweets.pdf
Fuma: but you can test this yourself!
Fuma: with AMD CodeXL, you can get the occupency statistics for a given shader code, and even see the GCN assembly for various architectures
Fuma: More info on occupancy: https://gpuopen.com/optimizing-gpu-occupancy-resource-usage-large-thread-groups/
perturbed: the 16 limit is for varying variables?
Fuma: command line tool for seeing various things about what the driver turns the shaders into: https://github.com/GPUOpen-Tools/RGA (goes all the way down to ASM)

Fuma: on the change to the texcoords:
Fuma: this gets a bit awkward because you can only set tcMod in a stage
illwieckz: yes I know of this bug
Fuma: If it wasn't for that, I would have made the change years ago
illwieckz: I think we have to consider that our engine is currently buggy on that part
illwieckz: I see the bug on solarium map even with separate texcoords
Fuma: bug?
illwieckz: the water diffuse is shifted
illwieckz: while the water normal is not
illwieckz: I have some ideas to revamp that part of the shader code
Fuma: oh
Fuma: does the q3shader have the same tcmod for both stages?
illwieckz: I don't know
Fuma: if it only puts it on the diffuse stage, then that is expected
Fuma: it is dumb design, but not a bug
illwieckz: but since diffuse/normal/specular/glow are just components of same material
Fuma: yeah
Fuma: that is why it is dumb design
illwieckz: I think about having an implicit "collapse" stage
Fuma: normal/specular/glow were implemented as shader stages on the q3shader system
Fuma: IIRC, this originates from doom3, then implemented in xreal
illwieckz: yes
illwieckz: diffuse/normal/specular would be automatically added to that "collapse" stage
illwieckz: instead of collapsing them at the end of parsing
Fuma: except glow, glow was added by me so we didn't have to vertex skin and render our alien models twice
Fuma: was just stage colormap with blendfunc add before
illwieckz: this would also allows to do terrain mapping with normal/specular
illwieckz: yep
illwieckz: so basically I see no one issue in that texcoords stuff given the current behavior is meh
illwieckz: and implementing that texcoords thing makes easier to implement a better behavior
Fuma: yeah
illwieckz: where the tcmod would be for the whole material
illwieckz: so you think this change is the way to go?
Fuma: yep

perturbed: so we should document that specular/glow/etc are pseudo-stages which can only modify the diffuse stage and don't stand on their own
illwieckz: perturbed, yes
Fuma: I wonder if it would be better to have something like:
Fuma> { stage diffuseMap; map ; normalMap ; specularMap <specularImage:; /* various other things here / }
Fuma: which currently isn't possible
Fuma: It would certainly be clearer
illwieckz: I think about this but:
illwieckz> { stage collapseStage; diffuseMap ; normalMap ; specularMap <specularImage:; /
various other things here */ }
Fuma: yeah
illwieckz: this is what I was talking about above
Fuma: :)
illwieckz: and having that collapseStage implicit and empty, so if there diffuseMap/normalMap/… keywords at the root of the shader, it's automatically added
illwieckz: keeps backward compatibility and it's ok because of concision
Fuma: oh
perturbed: i don't think there should be another way to do it unless you are committed to getting rid of the old one
illwieckz: Fuma, see what I wrote there: #184 (comment)
illwieckz: perturbed, my idea would be backward compatible

@slipher

This comment has been minimized.

Copy link
Contributor

slipher commented Mar 31, 2019

LGTM

use only one texCoords per surface
the extra compute and boilerplate code seem useless and it already
ended in bugs when parallax offset was applied to diffuse, normal,
specular but nor glow map, and I would find stupid to add more
boilerplate when height map will be implemented

diffuse/normal/specular/glow/height maps are component
of the same material, so there is no reason to handle
their coordinates separately

doing this would also avoid the need for a lot of ifdef code
when normal/specular/glow/parallax support is disabled

and well, that extra code seems useless even when they are enabled

it unify the symbol names to var_TexCoords

alongside the var_TexDiffuse, var_TexNormal, var_TexSpecular,
var_TexGlow there was some occurrence of var_Tex

some comments in already rewritten code told that Tr3b had issue
with some driver/hardware suffered from "too much var" issue when
passed from vp to fp shaders, leading him to abuse some variables,
see 72268b, even if I have not faced this issue with hardware that
is so old the game is unplayable, this is an even better way to
reduce the number of variables passed from vp to fp

for multimap material (diffuse/normal/specular/glow), the texCoords
used is the diffuse one

note that we already computed the parallax texOffset from diffuse coords
only, and applied it to other maps as it would be stupid and inefficient
to compute the parallax texOffset on each normal/specular/glow coords

@illwieckz illwieckz force-pushed the illwieckz:texcoords branch from 4079b72 to fb8b561 Mar 31, 2019

@illwieckz illwieckz merged commit fb8b561 into DaemonEngine:master Mar 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.