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

tr_shader: do not detect heightmap in normalmap alpha channel, unless asked to, fix #375 #379

Merged
merged 1 commit into from Oct 14, 2020

Conversation

illwieckz
Copy link
Member

With this PR,

This code also introduces an internal ParseNormalMapDetectHeightMap function that attempts to detect an alpha channel and declare it's an heightmap. There is no material keyword for that and it must not be implemented as a keyword.

This internal function is only purposed to be used by compatibility code like the Darkplaces compatibility mode. Darkplaces-based games being known to not use formats that would break on detection, it's seems to be safe to use this detection function for their assets.

@illwieckz illwieckz added this to To do in Unvanquished via automation Oct 4, 2020
@illwieckz illwieckz added this to To do in Xonotic via automation Oct 4, 2020
@illwieckz
Copy link
Member Author

@slipher do you have any remark on it? I would appreciate some remark on pointer wizardry to keep an unmodified copy of **text if there is a better implementation possible.

@slipher
Copy link
Member

slipher commented Oct 12, 2020

I suggest as follows. Notes:

  • DoDebugCode is used to execute the code only if the log level is debug.
  • char buffer[1024] = ""; requires the entire buffer to be zeroed.
	const char* initialText = *text;

	ParseNormalMap( stage, text, bundleIndex );

	/* Tell renderer to enable relief mapping since an heightmap is found
	also tell renderer to not abuse normalmap alpha channel because it's an heightmap.
	See https://github.com/DaemonEngine/Daemon/issues/183#issuecomment-473691252 */
	if ( stage->bundle[ bundleIndex ].image[ 0 ]->bits & IF_NORMALMAP
		&& stage->bundle[ bundleIndex ].image[ 0 ]->bits & IF_ALPHA )
	{
		Log::defaultLogger.DoDebugCode([&] {
			char buffer[ 1024 ];
			buffer[ 0 ] = '\0'
			bool parsed = ParseMap( &initialText, buffer, sizeof( buffer ) );
			ASSERT( parsed );
			Log::Debug("Found heightmap embedded in normalmap '%s'", buffer);
		});

		stage->isHeightMapInNormalMap = true;
	}

@illwieckz illwieckz force-pushed the explicitheightmap branch 2 times, most recently from 425657e to d60ede3 Compare October 13, 2020 03:05
@illwieckz
Copy link
Member Author

@slipher, I rewrote the things accordingly. I added a UNUSED keyword to silent the compiler complaining about parsed being unused on non-debug code. Or I may hack something like that instead:

			#ifdef DEBUG
			bool parsed = 
			#endif
			ParseMap( &initialText, buffer, sizeof( buffer ) );

@slipher
Copy link
Member

slipher commented Oct 13, 2020

There is already a facility for marking an unused variable, Q_UNUSED. It's used like Q_UNUSED(variable);

Alternatively if(!parsed) ASSERT(false); would work.

@illwieckz
Copy link
Member Author

Hmm, since all those tricks are there to move the ParseMap call out of ASSERT, then we can just avoid a bool and do:

			if ( !ParseMap( &initialText, buffer, sizeof( buffer ) ) )
			{
				ASSERT( false );
			}

@illwieckz illwieckz merged commit ced56d8 into DaemonEngine:0.52.0/sync Oct 14, 2020
Xonotic automation moved this from To do to Done Oct 14, 2020
Unvanquished automation moved this from To do to Done Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Unvanquished
  
Done
Xonotic
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants