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

broken rotating texture #35

Closed
illwieckz opened this issue Sep 28, 2017 · 28 comments

Comments

6 participants
@illwieckz
Copy link
Member

commented Sep 28, 2017

See these two videos: 1, 2, two fans are rotating wrongly in parpax map, while the others are rotating as expected. For example in the ventilation hall, the one near the tank room is wrong, but not the other one in the same hall.

There is one crazy fan in ventilation hall (near tank room):

crazy fan

crazy fan

And one in ventilation (near alien base):

crazy fan

These fans look to just be spinning textures. I noticed similar rotating fan bug in some other maps like the metro map by KOsAD. The same map has correct rotating fan while loaded on Tremulous and has crazy fan while loaded on Unvanquished. That's why I think it's a renderer bug.

Parpax used to have more broken fans but these fan were removed once some geometry was redone. This bug is very old (I always seen it).

@Viech

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

I'm pretty sure that at least the fan in the first two pictures worked on my end, and I cannot think of a way how I could have broken it. So I would also assume this to be a renderer bug.

@illwieckz illwieckz changed the title parpax crazy fans broken rotating texture Oct 23, 2018

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Oct 23, 2018

This is an engine issue, the issue got confirmed with a Smokin' Guns map, the train one:

See how looks the closest wheel from this point of view:

Train

See how broken looks the opposite wheel from this point of view:

Train

Of course the train wheel looks good on ioquake3, so it's an engine issue.

It's possible to download some preview dpks of Smokin' Guns assets this way:

wget -O- http://gg.illwieckz.net/dl/smokinguns/dev/SmokinGunsAssets.get | xargs wget

Then to load the map in Unvanquished this way (requires g_neverEnd since the map lacks some Unvanquished required entities):

set g_neverEnd 1
devmap dm-train
@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Oct 24, 2018

pinging @gbougard 😉

@illwieckz illwieckz added this to To Do in Smokin' Guns Oct 24, 2018

@illwieckz illwieckz added this to To do in Unvanquished Oct 24, 2018

@t4im t4im added the Renderer label Nov 24, 2018

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 24, 2019

I have been examining this issue and I think something similar happens on the working fans as well.

The choppy rendering is due to conversion into a .gif in the actual engine this is completely smooth.

Below is the broken one slowed down.

BrokenFan

But note the corners in the working slowed fan below. This fan is directly underneath 'the one crazy fan' seen above.

WorkingFan

All the fans seem to be have multiple fans spinning around them. This was also observed with the fans in the ventilation duct as well.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Yeah, I noticed that, it looks like the center of rotation is wrong on "crazy fan".
Thanks for the self-explaining animations!

Edit: I think the fact textures are tiled infinitely is expected, hence the texture repeating and displaying other fans in corner because there is not enough transparent surface around the fan.

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 24, 2019

You can see this in the engine by shifting the rotation speed of textures/parpax_evillair/e6metalfan_blade in the parpax_evillair.shader file up from -1500 to -100.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Thanks, then we can guess the issue is in tcmod rotate implementation or next to it.

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 24, 2019

Where would that code be found? Only a file name... That question is way too specific otherwise.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

That shader keyword is parsed in src/engine/renderer/tr_shader.cpp,

// rotate
else if ( !Q_stricmp( token, "rotate" ) )
{
token = COM_ParseExt2( text, false );
if ( token[ 0 ] == 0 )
{
Log::Warn("missing tcMod rotate parms in shader '%s'", shader.name );
return false;
}
tmi->rotateSpeed = atof( token );
tmi->type = texMod_t::TMOD_ROTATE;
}

The bug has high chance to be between that step and the rendering itself, or somewhere in some general purpose texture code, since it looks like we have a coordinate issue.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

$ grep -r TMOD_ROTATE src
src/engine/renderer/tr_shade_calc.cpp:			case texMod_t::TMOD_ROTATE:
src/engine/renderer/tr_shade_calc.cpp:			case texMod_t::TMOD_ROTATE2:
src/engine/renderer/tr_shader.cpp:		tmi->type = texMod_t::TMOD_ROTATE;
src/engine/renderer/tr_shader.cpp:			tmi->type = texMod_t::TMOD_ROTATE2;
src/engine/renderer/tr_local.h:	  TMOD_ROTATE,
src/engine/renderer/tr_local.h:	  TMOD_ROTATE2
@Rootyjr

This comment has been minimized.

Copy link

commented Jun 24, 2019

In tr_shade_calc.cpp


case texMod_t::TMOD_ROTATE:
{
	x = -bundle->texMods[ j ].rotateSpeed * backEnd.refdef.floatTime;

	MatrixMultiplyTranslation( matrix, 0.5, 0.5, 0.0 ); <-Changing these values seems to have a direct impact on how the fans render.
	MatrixMultiplyZRotation( matrix, x );
	MatrixMultiplyTranslation( matrix, -0.5, -0.5, 0.0 ); <-Changing these values seems to have a direct impact on how the fans render.
	break;
}
@Rootyjr

This comment has been minimized.

Copy link

commented Jun 24, 2019

Do you know why there is a TMOD_ROTATE1 and TMOD_ROTATE2?

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

The code in src/engine/renderer/tr_shade_calc.cpp is there since the beginning of the repository, so either the bug is inherited from XreaL, either the code is elsewere.

Note: ioq3 code is different (split between code/renderergl2/tr_shade.c and code/renderergl2/tr_shade_calc.c, and it looks less abstracted).

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

If I uglily backport this parpax map to run on Unvanquished 0.6.0 (the oldest build I have), the fan is not broken. I also know that the map was already broken in Unvanquished 0.37.0. So the bug was introduced between 0.6 and 0.37.

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 24, 2019

In the 0.6 version of the game both the current code existed and some older code existed as part of a some earlier renderer.

void RB_CalcRotateTexCoords( float degsPerSecond, float *st )
{
	float        timeScale = tess.shaderTime;
	float        degs;
	int          index;
	float        sinValue, cosValue;
	texModInfo_t tmi;

	degs = -degsPerSecond * timeScale;
	index = degs * ( FUNCTABLE_SIZE / 360.0f );

	sinValue = tr.sinTable[ index & FUNCTABLE_MASK ];
	cosValue = tr.sinTable[( index + FUNCTABLE_SIZE / 4 ) & FUNCTABLE_MASK ];

	tmi.matrix[ 0 ][ 0 ] = cosValue;
	tmi.matrix[ 1 ][ 0 ] = -sinValue;
	tmi.translate[ 0 ] = 0.5 - 0.5 * cosValue + 0.5 * sinValue;

	tmi.matrix[ 0 ][ 1 ] = sinValue;
	tmi.matrix[ 1 ][ 1 ] = cosValue;
	tmi.translate[ 1 ] = 0.5 - 0.5 * sinValue - 0.5 * cosValue;

	RB_CalcTransformTexCoords( &tmi, st );
}

It looks very similiar to the code used by smokinguns

void RB_CalcRotateTexMatrix( float degsPerSecond, float *matrix )
{
	float timeScale = tess.shaderTime;
	float degs;
	int index;
	float sinValue, cosValue;

	degs = -degsPerSecond * timeScale;
	index = degs * ( FUNCTABLE_SIZE / 360.0f );

	sinValue = tr.sinTable[ index & FUNCTABLE_MASK ];
	cosValue = tr.sinTable[ ( index + FUNCTABLE_SIZE / 4 ) & FUNCTABLE_MASK ];

	matrix[0] = cosValue; matrix[2] = -sinValue; matrix[4] = 0.5 - 0.5 * cosValue + 0.5 * sinValue;
	matrix[1] = sinValue; matrix[3] = cosValue;  matrix[5] = 0.5 - 0.5 * sinValue - 0.5 * cosValue;
}
@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

Oh yeah there was multiple renderer, perhaps one was buggy and not the other one, I don't know.

In anyway the fan rotation is not broken on Unvanquished 0.18.0, but it still has the two renderers.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

In fact I'm using GL3 renderer in both 0.06 and 0.18, hence the current code path, and the fan is not broken, so the bug was introduced after 0.18.

Edit: Bug was introduced between 0.18.0 (bug not there) and 0.32.0 (bug there).

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

OK, bug was not there in 0.29.0 and was there in 0.30.0, it was introduced between those two releases.

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 25, 2019

Sorry for the poor lighting.

Below is a gif of the broken vent duct fan:

BrokenFan

Below is a gif of the 'working' vent duct fan:
WorkingFan

Usually the working one works when we translate using the code below

					MatrixMultiplyTranslation( matrix, 0.5, 0.5, 0.0 );
					MatrixMultiplyZRotation( matrix, x );
					MatrixMultiplyTranslation( matrix, -0.5, -0.5, 0.0 );

The code that produces the current results is

					//MatrixMultiplyTranslation( matrix, 0.5, 0.5, 0.0 );
					MatrixMultiplyZRotation( matrix, x );
					//MatrixMultiplyTranslation( matrix, -0.5, -0.5, 0.0 );

I believe the error that we are seeing may be caused by the rotation of shader / texture / brush when the map was created. I assume that the old code properly handled this and ignored orientation, but I can't say. But based on this I believe that if the translation values were altered into reverse the broken fans would work and the working fans would become 'crazy'

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 25, 2019

As I suspected changing the code to what is seen below will fix the typically crazy fan in the duct. While making the typically normal fan in the duct become crazy.

MatrixMultiplyTranslation( matrix, 0.5, -0.5, 0.0 );
MatrixMultiplyZRotation( matrix, x );
MatrixMultiplyTranslation( matrix, -0.5, 0.5, 0.0 );
@megatog615

This comment has been minimized.

Copy link

commented Jun 25, 2019

I have been examining this issue and I think something similar happens on the working fans as well.

The choppy rendering is due to conversion into a .gif in the actual engine this is completely smooth.

Below is the broken one slowed down.

BrokenFan

But note the corners in the working slowed fan below. This fan is directly underneath 'the one crazy fan' seen above.

WorkingFan

All the fans seem to be have multiple fans spinning around them. This was also observed with the fans in the ventilation duct as well.

Wouldn't clamping fix this issue? This can be enabled in the shader(mtr?) definition file.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 25, 2019

@megatog615 Maybe clamping would fix the second issue, but this one is not really problematic: even if it's not realistic, what is expected to be rendered is rendered, even if it's wrong.

@Rootyjr what you discovered makes me think that perhaps some code elsewhere in dæmon was simplified or rewritten without some hacks required for some corner-cases. I doubt the problem is in that transform code since this transform code looks very old, the bug is probably already triggered when the execution comes to that point.

@Rootyjr

This comment has been minimized.

Copy link

commented Jun 26, 2019

Exactly my thoughts. I am now thinking our bug is in tr_backend or whereever the matrix object is implemented for each texture/brush.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

It looks like the bug was introduced by the vertexpack merge on 2014-08-08
initial tree: Unvanquished/Unvanquished@9501270

extracted engine tree: 5ea5314

I'm not able to run custom build of 0.30.0 or that commit anymore but I know that 0.30.0 has the bug since I reproduce the bug using old 0.30.0 build and current map, and there is no engine-related commit between this merge and the 0.30.0 release.

I'm able to build and run the commit in master branch that precedes the merge of the vertexpack one, and the bug is not there yet.

Commit that precede that merge and does not display the bug yet (initial tree): Unvanquished/Unvanquished@67d4839

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

I will have hard time to bisect this since I'm not able to run anything in that vertexpack branch: some librocket code is faulting on invalid pointer freeing.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

@gimhael, It looks like you contributed a lot to that vertexpack branch so you are probably among the ones who understand the most that code, do you have any idea about what can be the cause of that bug?

@gimhael

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

The cause is that the vertexpack engine tries to optimize texture coords, because fp16 numbers have 1/2048 resolution only between -1.0 and 1.0, and so we could have several texel rounding errors if the coords are too large.

The optimization finds connected surfaces and then shifts the texture coords so that their average value is near 0. If the range is 0-1, there are two equally good solutions, either keep the range 0-1 or shift to -1-0, and in this case tiny rounding errors decide which one is used. For non-animated textures this is no issue, but the rotating textures always rotate around 0.5/0.5, and if the texture coords are shifted, it shifts the rotation center too.

The easy fix is to move the average as close as possible to 0.5 instead of 0.0, then all standard 0-1 textures will not be shifted.


diff --git a/src/engine/renderer/tr_bsp.cpp b/src/engine/renderer/tr_bsp.cpp
index 95acd29..8a3ffcb 100644
--- a/src/engine/renderer/tr_bsp.cpp
+++ b/src/engine/renderer/tr_bsp.cpp
@@ -955,7 +955,7 @@ static void ParseFace( dsurface_t *ds, drawVert_t *verts, bspSurface_t *surf, in
 	for( i = 0; i < numVerts; i++ ) {
 		if( components[ i ].minVertex == i ) {
 			for( j = 0; j < 2; j++ ) {
-				components[ i ].stBounds[ 0 ][ j ] = rintf( 0.5f * (components[ i ].stBounds[ 1 ][ j ] + components[ i ].stBounds[ 0 ][ j ]) );
+				components[ i ].stBounds[ 0 ][ j ] = rintf( 0.5f * (components[ i ].stBounds[ 1 ][ j ] + components[ i ].stBounds[ 0 ][ j ]) - 0.5f );
 			}
 		}
 

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2019

Awesome, you fixed it!

🍾 💯 🎉

parpax fan

train fan

metro fan

illwieckz added a commit to illwieckz/Daemon that referenced this issue Jul 1, 2019

illwieckz added a commit to illwieckz/Daemon that referenced this issue Jul 3, 2019

illwieckz added a commit to illwieckz/Daemon that referenced this issue Jul 6, 2019

illwieckz added a commit to illwieckz/Daemon that referenced this issue Jul 6, 2019

illwieckz added a commit to illwieckz/Daemon that referenced this issue Jul 8, 2019

Unvanquished automation moved this from To do to Done Jul 8, 2019

Smokin' Guns automation moved this from To do to Done Jul 8, 2019

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.