Iterate shader stages directly, avoid array of pointers

[illwieckz]

Iterate shader stages directly, avoid array of pointers.

[illwieckz]

The code iterate an array of MAX_SHADER_STAGES stages with numStages only when parsing shaders, as long as we can disable some and group them.

When making the shader permanent and ready to feed the renderer, we now only allocate the exact amount of active stages just before copying them to the permanent shader.

Then, at render time, the code now iterate pointers directly, and we avoid dereferencing the pointer of a pointer on every stage.

[illwieckz]

Why do I get that?

AppVeyor was unable to build non-mergeable pull request

[DolceTriade]

Rebase maybe?

[illwieckz]

Rebase maybe?

At the time it was rebased on current master, I wonder if AppVeyor doesn't build if there are unsolved blocking discussions.

[DolceTriade]

C:\projects\daemon\src\engine\renderer\tr_shader.cpp(4751,10): error C2220: the following warning is treated as an error [C:\projects\daemon\build\client-objects.vcxproj]
C:\projects\daemon\src\engine\renderer\tr_shader.cpp(4751,10): warning C4389: '!=': signed/unsigned mismatch [C:\projects\daemon\build\client-objects.vcxproj]

[illwieckz]

C:\projects\daemon\src\engine\renderer\tr_shader.cpp(4751,10): error C2220: the following warning is treated as an error [C:\projects\daemon\build\client-objects.vcxproj]
C:\projects\daemon\src\engine\renderer\tr_shader.cpp(4751,10): warning C4389: '!=': signed/unsigned mismatch [C:\projects\daemon\build\client-objects.vcxproj]

This one is new!!! 😀️

I now fixed it.

[illwieckz]

I added a commit (to be squashed) that copies the stage data in one go. That's something I tried before but it didn't worked, I probably made a stupid mistake at the time because right now it works.

[illwieckz]

I added some fixup commits to be squashed to simplify the code.

[illwieckz]

if ( shader->defaultShader || !shader->stages ) I'm saying that it looks suspicious; provokes a "WTF" from the reader.

if ( numStages ) No need for an if. The functioning should be consistent zero or nonzero

About setting stages to nullptr to know if the stage list is empty, the code was already doing that for both the stage list and the shader:

if ( surfaceShader != nullptr )
{
	state = ( surfaceShader->remappedShader ) ? surfaceShader->remappedShader : surfaceShader;

	tess.surfaceShader = state;
	tess.surfaceStages = state->stages;
	tess.surfaceLastStage = state->lastStage;

	Tess_MapVBOs( false );
}
else
{
	state = nullptr;

	tess.surfaceShader = nullptr;
	tess.surfaceStages = nullptr;
	tess.surfaceLastStage = nullptr;
	Tess_MapVBOs( false );
}

// …

if ( state != nullptr && state->isSky )
{
	tess.stageIteratorFunc = &Tess_StageIteratorSky;
}

And even after my patches the code still expects the shader to be nullptr if empty.

So I would not be against setting stages to nullptr when empty, and just testing for stages not being null instead of comparing stages with lastStage, really.

Even my latest changes simplifying the code would carry the nullptr. And we can skip calling the allocation and copy functions of the stage list with an if, even if the code works without.

[illwieckz]

Any last word?

[slipher]

Any last word?

I don't see any LGTMs 🤪

[illwieckz]

Any last word?

I don't see any LGTMs 🤪

That can be a last word.

[illwieckz]

I would like to know if there is something blocking there. I'm planning other changes in the shader code and I want to base them on this.

Among the things remaining in the discussions I see the disliking of the global variable numStages not being global while not being part of a struct. First thing a disliking is not blocking. Second thing, it is now only global to tr_shader.cpp and not shared with all the code and especially not used anymore in tr_shade.cpp and maybe others. It is also now only used for the temporary shader parsing data before making the permanent shader to be used for rendering things. It's actually a step forward to make possible in the future to transform the stage list into a vector or other structure if we prefer that to a “start,end” tuple, most of the code is now properly shaped to make other things possible. As a second step after this PR I also want to make this numStages to be part or a temporary struct in tr_shader.cpp, only used when parsing.

After this is done I have some refactor of the FinishShader() code to make it more readable and better understandable, along with moving some shader checking code to a dedicated function and reduce boilerplate. Doing that before this is merged would just create useless merge conflicts.

[illwieckz]

Well, a vector is probably a bad idea because we would drop the Hunk allocator, so this is probably already the closest to a vector we can do with an Hunk allocator.

[slipher]

LGTM to renderer: iterate shader stage array directly, avoid array of pointers and tr_shader: also list shaders without stages

I have not reviewed the other one.

[illwieckz]

Remaining unmerged commit is now to be reviewed there:

• https://github.com/DaemonEngine/Daemon/pull/1165