StrobeAPI Implementation #353

fuzun · 2018-02-18T16:24:43Z

Pull request for the v1: #343
Discussion: https://forums.blurbusters.com/viewtopic.php?f=7&t=3815

News (v3):

Complete refactor due to OOP implementation.
Now developers can develop their own Strobe implementation based on Strobe api (by deriving StrobeAPI abstract class).
Multiple Strobe implementations can work simultaneously.
Bug fixes.
...

News (v2):

Rewritten from scratch.
Supports phase matching.
Burn-in prevention. -> r_strobe_swapinterval .
New debug mode! (turn on with r_strobe_debug <0|1>)
Brightness reduction statistics in debug stats.
PWM (Simulated) statistics in debug stats.
Experimental "Badness" function in debug stats.
Some bug fixes.
Stability analyzing to auto disable strobing (r_strobe_cooldown ) - stats in debug stats.
...

Todo:

Making menu entry for enabling strobe mode.
Implement high precision timer to keep the internal phase on track with monitor. (In case of freezes etc.) * Thanks @mdrejhon .
Make swapping transition seamless by rendering non-opaque frames. * Thanks @mdrejhon .
~~Move "Strobe" to separate files.~~

Sample screenshot from the debug mode: https://camo.githubusercontent.com/081e3641e118be7554e42f54149dc9ce5c862724/68747470733a2f2f7667792e6d652f54646634675a2e706e67

This pull request will be edited and updated! It stands for review now. (9.03.2018)

Shows PWM stats in debug mode.

This reverts commit faa97ce.

a1batross · 2018-02-18T18:12:06Z

Great, I will review it ASAP.

a1batross · 2018-02-18T21:17:56Z

engine/client/cl_scrn.c

@@ -57,13 +57,22 @@ void SCR_DrawFPS( void )
 	static int	minfps = 9999;
 	static int	maxfps = 0;
 	double		newtime;
-	char		fpsstring[64];
+	char		fpsstring[2048];	//char fpsstring[64]; // Heap allocation if 2048 too much ?


I recommend you moving this code to separate function, so SCR_DrawFPS stays only for showing FPS information. One function does one thing.

It would be good to have OOP used here but since it can't be, copy-paste will be required for most of the SCR_DrawFPS code (to the new function). I would not favor copying and pasting because it makes it spaghetti-code but I could not find another good solution.

OOP is possible for C too. Partially.

I planned to share FPS stats between functions, so net_graph can show precise FPS too, instead of silly 1 / host.frametime.

So we would have a function for FPS calculation(current, best, worst and average), which writes result to public structure, and it can be used by "informative" code, like DrawFPS or Network Graph.

a1batross · 2018-02-18T21:21:07Z

engine/client/cl_scrn.c

+
+				strobeInterval = r_strobe->integer;
+
+				Q_snprintf(fpsstring,


You may want to use Con_NXPrintf here to avoid fpsstring overrun and keeping code simple.

Look to s_main.c:1607(around s_show->integer check) for example.

a1batross · 2018-02-18T21:23:16Z

engine/client/gl_local.h

@@ -349,6 +349,72 @@ void R_LightForPoint( const vec3_t point, color24 *ambientLight, qboolean invLig
 int R_CountSurfaceDlights( msurface_t *surf );
 int R_CountDlights( void );

+
+/// <STROBE> // Move to strobe.h ?


Yeah, it would be a good idea.

Because all source code is categorized, you may want to move code to "r_strobe.c" and "r_strobe.h" files.

a1batross · 2018-02-18T21:25:38Z

engine/client/gl_local.h

+{
+	float x, y, w, h;
+} _rectf_t;
+extern _rectf_t con_rect;


Such generic definitions may be moved to "client.h" header, as con_rect can be used by other parts of engine.

That would also require moving the _rectf definition to more generic header (client.h in this case). I did not do that because I intended to use _rectf only for strobe internal purposes.

I am not familiar with Xash3D so,

I am not sure if this definition would be ok to be in a more generic header for public / general uses.

There may also be a float / double rectangle struct definition in the engine that I could not find as it is pretty basic thing. I have found similar wrect but it was int.

It's just doesn't look right. I don't know can it be used elsewhere than strobing code. If it's intended to be private, you may move extern "_rectf_t" to file with your strobing code and don't show it for everyone(I mean, for code, yes :))

a1batross · 2018-02-18T21:26:11Z

engine/client/gl_local.h

+void R_Strobe( void );
+
+typedef enum {
+	p_positive = 1, // Phase: Positive


We have BIT() macro for eaiser bitmasks.

Marked. Do we have more generic handling for flags or is it only this BIT macro?

No, it's only BIT macro.

Unkle Mike refactored it. He added FBitSet() for bit checking, ClearFlag, SetFlag and ResetFlag macros. But fork still don't moved on his version.

a1batross · 2018-02-18T21:28:20Z

engine/client/gl_local.h

+	fstate_e frameInfo; // Frame info
+} StrobeInfo_t;
+
+extern StrobeInfo_t StrobeInfo;


If this defintions are not used by other parts of game engine, it's better to keep them encapsulated.

Encapsulating by moving to separate header file dedicated to strobing? If that is the case, I agree.

Just as you want.

It's not limited to headers. You maybe want to define them in *.c file and declare as "static", so they wouldn't accessible from other compilation units.

a1batross · 2018-02-18T21:41:13Z

engine/client/gl_local.h

+#define STROBE_DEVIATION_SIZE 60
+
+// Experimental 'Badness' function
+#define STROBE_BADNESS( diffP, diffNs ) \


Macros are one of the greatest thing from C language, but they are also may cause not expected bugs.

In this case, you may pass something like: _calculatePercentage( 2 + 2, 1 ) and this will be expanded to: ( 100 * 2 + 2 / 1 ), leaving you with incorrect result.

I recommend to move them to _inline functions or just put macro arguments to brackets. For example:
#define _calculatePercentage( x, y ) ( 100 * ( y ) / ( x ) )

Another thing is that it will be hard to read such code and debug, so inlined functions is preferred. And you will get a free type checking! :)

I wanted to make them as inline functions at first place, but because of there is no separate c file for strobe I used macros as it would be bad to put these internal functions into gl_rmain.

If you approve separating the "strobe", which in my opinion is the way to go, I will do that but I am not into project files like cmake files so I may not be able to integrate separate files into the project files properly.

CMake and Makefiles both do recursive searching of any *.c files.

Except is Android.mk, but it's not too hard. Just add path to file at the end of LOCAL_SRC_FILES and don't forget reverse slash at the end of previous line.

a1batross · 2018-02-18T21:43:00Z

engine/client/gl_rmain.c

-	int getInterval = r_strobe->integer; // Check through modified tag first?
+	static int strobeInterval = 0;
+	static int swapInterval = 0;
+	static double recentTime = 0.0, recentTime2 = 0.0;


static variables is always initialized to zero. It is required by C standard.

a1batross · 2018-02-18T21:44:37Z

engine/client/gl_rmain.c

+	swapInterval = r_strobe_swapinterval->integer;
+
+	if ( (strobeInterval == 0) ||
+		((gl_swapInterval->integer == 0) && (strobeInterval != 0)) )


Integers are always FALSE when equal to zero and TRUE otherwise.

I always try to be more explicit, but I noted it.

It isn't bad. Just more readable for typical C or C++ programmer, I think.
It will not readable for Java programmers. :D :D

a1batross · 2018-02-18T21:46:37Z

engine/client/vid_common.c

+	r_strobe_debug = Cvar_Get( "r_strobe_debug", "0", CVAR_ARCHIVE, "show strobe debug information" );
+	r_strobe_cooldown = Cvar_Get( "r_strobe_cooldown", "3", CVAR_ARCHIVE, "strobe cooldown perios in secs" );
+
+	StrobeInfo.pCounter = 0; StrobeInfo.pBCounter = 0; StrobeInfo.pNCounter = 0;


You may use Q_memset() here. Also global variables is always zero initialized. It's a standard too.

fuzun · 2018-02-22T21:05:25Z

In maintenance...
Will be reopened.

Sync with the base

Sync with master

… strobe-support

a1batross · 2018-04-07T01:28:48Z

Just comment here, when it will be ready.

I also will merge it for upcoming code base sync with Xash3D 1.0.

fuzun · 2018-04-07T02:23:03Z

@a1batross Will FPS be global ? You have mentioned about flag operations, does version 1.0 contain new macros for that purpose ? And I have used some reserved keywords like "this" thinking it would not matter since it is a c file and therefore must be treated as c code, should I revert it to _this ?

Also will white color be added to the color table? I think it can be added after the "default" color (+1 increased array size).

This reverts commit 84759c7.

… strobe-support

Update

fuzun · 2018-05-07T19:17:51Z

https://github.com/fuzun/strobe-api

Sync with head fork

… strobe-support

fuzun · 2018-06-30T15:44:56Z

How to hook a StrobeAPI derived work (StrobeAPI implementation) into xash3d ?

in cl_view.c V_PostRender function, this part can be modified:

#ifdef STROBE_ENABLED
		// if ( STROBE_TEMPLATE )
		//  STROBE_TEMPLATE->STROBE_IMPL_FUNC_DEBUGHANDLER( &STROBE_TEMPLATE );
		if ( STROBE_CORE )
			STROBE_CORE->STROBE_IMPL_FUNC_DEBUGHANDLER( &STROBE_CORE );
#endif

Debug handling function (printing informative strobe debugging text output from StrobeAPI) of your Strobe implementation can be called here.

in gl_rmain.c R_EndFrame function, this part can be modified:

#ifdef STROBE_ENABLED
// StrobeAPI.Invoker( STROBE_INVOKE(STROBE_TEMPLATE) );
StrobeAPI.Invoker( STROBE_INVOKE(STROBE_CORE) );
#else

Check r_strobe_template.c.TEMPLATE and r_strobe_template.h.TEMPLATE files. They are templates for creating a new StrobeAPI implementation. They can be used as a base and for actual implementation Strobe-Core (r_strobe_core.h and r_strobe_core.c) can be inspected.

How StrobeAPI and StrobeAPI implementations exposed to xash3d ?

A structure which contains console variables and Invoker, Constructor and Destructor functions are exposed through "StrobeAPI" instance of the structure.
R_InitStrobeAPI( ) from StrobeAPI for registering console variables is exposed.
Constructor, destructor, reinit, main functions from StrobeAPI implementations are exposed while being guarded by StrobeAPI macros.
Bunch of macros starting with STROBE or _STROBE from StrobeAPI are exposed.
Bunch of structures starting with STROBE or StrobeAPI_ from StrobeAPI are exposed.
An implementation structure and its instance from StrobeAPI implementation are exposed while being guarded by StrobeAPI macros.
StrobeAPI and StrobeAPI implementation generic functions and private variables etc. are NOT exposed.
Contents of r_strobe_api_protected_.h are not exposed.
If STROBE_DISABLED is defined, nothing is exposed.

Console variables

r_strobe : For setting Strobe method. 1: NORMAL - BLACK , 2 : NORMAL - BLACK - BLACK, -3: BLACK - BLACK - BLACK - NORMAL . Set 0 to disable strobing.
r_strobe_swapinterval : For setting phase swap interval of Strobing. (in seconds). May cause image retention if set to 0 in some monitors (0 disables phase swapping).
r_strobe_debug : For informing StrobeAPI instance to do something with StrobeAPI informative debug output text. Strobe-Core implementation shows the output text in right-up corner of the screen. (0 or 1)
r_strobe_cooldown : When standard deviation of collected FPS values in a certain time period exceeds the determined limit, strobing gets disabled for a time period. This cvar sets the time period of how long strobing is disabled when there is instability. Should be set 0 to disable instability induced strobing cooldown.
deviation limit (not implemented): Standard deviation limit of FPS values collected in a time period.
count of collected fps values (not implemented): How many FPS values should be collected before calculating standard deviation.

These variables are common in all StrobeAPI implementations because they are defined in StrobeAPI. In future these variables should be defined in implementations instead of StrobeAPI.

a1batross · 2018-06-30T19:07:50Z

Good. I will test it ASAP and merge for old engine. New engine isn't ready for public and things may change, but I hope new engine will support strobing too.

I have a question, though. Will strobing help to defeat motion blur at 60 Hz? I got a new device recently, it's 60 Hz but too blurry.

fuzun · 2018-06-30T19:37:52Z

I never intended to make this C89 compatible but for some reason compiler that Travis using can not compile if variables are defined in for statements due to "C89 compatibility". It does not need variables to be defined at start of a block.

I have a 60 hz screen and I am not satisfied with the strobing performance. Because even with the least aggressive strobe method (1 or -1), effective fps drops to 60/2 = 30 fps and frequency of signal drops to 30 hz. I find 30 fps/hz to be not enough for games xash3d support. r_strobe -2 method makes effective fps 40 but signal frequency (I call it PWM simulation) drops to 20 hz (60 * 1/3). I think at 20 hz it should not be even called as "strobing".

When I overclock the screen to 75 hz, strobing does not become annoying. So I think 75hz might be bare minimum for strobing.

I have another 60 hz screen that I overclocked it to 108 hz. Strobing helps too much for eliminating motion blur however that screen has only 220 cd/m2 peak brightness and even with least aggressive strobe method, screen becomes too dark. It also had image retention problem but it is fixed with r_strobe_swapinterval setting thanks to @mdrejhon .

And the last monitor I tested strobing is 144 hz. It has built-in hardware strobing so effective fps does not drop. The game is playable even with aggressive strobe methods like r_strobe = 2, 3 or -2 . I think that performance of software strobe method is like 30-35% of the built in hardware strobing.

There is an utility called CRU (Custom resolution utility) for overclocking monitors. I suggest you to try it.

As for this pull request, there are a few things that need to be corrected. They are not critical so you can merge it in my opinion. When I have time I will try to update it.

fuzun added 12 commits February 10, 2018 21:32

Strobe Implementation v2

5c14dfa

Brightness reduction calculations

5a01798

PWM Simulation (Debug Mode)

511a8fe

Shows PWM stats in debug mode.

Sync with 8578012

faa97ce

Revert "Sync with 8578012"

1d58b1e

This reverts commit faa97ce.

Sync with 8578012

f6a61e8

Some typo correction

7739e2d

Refactor & PWM Simulation Updates

9d01da8

Merge branch 'master' of https://github.com/FWGS/xash3d into FWGS-master

03a3b9e

Merge branch 'FWGS-master' into strobe-support

c473483

MainUI Sync

48782be

Implemented Stability Checker & Refactoring

9a89b57

a1batross reviewed Feb 18, 2018

View reviewed changes

fuzun closed this Feb 22, 2018

fuzun added 3 commits March 4, 2018 01:02

Complete refactor due to OOP implementation

b2fe83e

Merge branch 'master' of https://github.com/FWGS/xash3d into FWGS-master

425f426

Merge branch 'FWGS-master' into strobe-support

6a4683a

fuzun reopened this Mar 3, 2018

fuzun changed the title ~~Software Based Strobing Implementation v2~~ Software Based Strobing Implementation v3 Mar 3, 2018

fuzun and others added 9 commits March 28, 2018 13:54

GCC Fix #2 ?

4db52f1

Merge pull request #3 from FWGS/master

465c428

Sync with the base

Record elapsed time & small fixes

bb5d065

Reduced macros

1c1be72

Fixes & Better oop

2bcc54f

Merge pull request #4 from FWGS/master

f1ffa67

Sync with master

Merge branch 'strobe-support' of https://github.com/fuzun/xash3d into…

72642be

… strobe-support

Adjusted constness & format

84759c7

Format

41524d7

fuzun mentioned this pull request Apr 11, 2018

Software Based Strobing Implementation (Black Frame Insertion) #343

Merged

fuzun added 8 commits April 13, 2018 20:33

Merge branch 'master' into strobe-support

472b8c0

Revert "Adjusted constness & format"

8944808

This reverts commit 84759c7.

Merge branch 'strobe-support' of https://github.com/fuzun/xash3d into…

1ccf81a

… strobe-support

Better transition for r_strobe = 1 or -1

b590a5b

Better statistics & transition fix

c372b1c

Change in stability measurement

2592dd9

Format

325d385

Merge pull request #5 from FWGS/master

254e43a

Update

fuzun added 4 commits June 30, 2018 17:15

Safer alpha value for transitions

419422e

Merge pull request #6 from FWGS/master

620d12b

Sync with head fork

Merge branch 'strobe-support' of https://github.com/fuzun/xash3d into…

3d5d0cb

… strobe-support

C89 compatibility

54aeb83

fuzun changed the title ~~Software Based Strobing Implementation v3~~ StrobeAPI Implementation Jun 30, 2018

a1batross merged commit 54aeb83 into FWGS:master Jul 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StrobeAPI Implementation #353

StrobeAPI Implementation #353

fuzun commented Feb 18, 2018 •

edited

Loading

a1batross commented Feb 18, 2018

a1batross Feb 18, 2018

fuzun Feb 18, 2018

a1batross Feb 18, 2018

a1batross Feb 18, 2018 •

edited

Loading

a1batross Feb 18, 2018

a1batross Feb 18, 2018

a1batross Feb 18, 2018

fuzun Feb 18, 2018 •

edited

Loading

a1batross Feb 18, 2018

a1batross Feb 18, 2018

fuzun Feb 18, 2018

a1batross Feb 18, 2018

a1batross Feb 18, 2018

fuzun Feb 18, 2018

a1batross Feb 18, 2018

a1batross Feb 18, 2018

fuzun Feb 18, 2018

a1batross Feb 19, 2018

a1batross Feb 18, 2018

a1batross Feb 18, 2018

fuzun Feb 18, 2018

a1batross Feb 19, 2018

a1batross Feb 18, 2018

fuzun commented Feb 22, 2018

a1batross commented Apr 7, 2018

fuzun commented Apr 7, 2018

fuzun commented May 7, 2018

fuzun commented Jun 30, 2018

a1batross commented Jun 30, 2018

fuzun commented Jun 30, 2018 •

edited

Loading

StrobeAPI Implementation #353

StrobeAPI Implementation #353

Conversation

fuzun commented Feb 18, 2018 • edited Loading

Pull request for the v1: #343 Discussion: https://forums.blurbusters.com/viewtopic.php?f=7&t=3815

a1batross commented Feb 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a1batross Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuzun Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fuzun commented Feb 22, 2018

a1batross commented Apr 7, 2018

fuzun commented Apr 7, 2018

fuzun commented May 7, 2018

fuzun commented Jun 30, 2018

How to hook a StrobeAPI derived work (StrobeAPI implementation) into xash3d ?

How StrobeAPI and StrobeAPI implementations exposed to xash3d ?

Console variables

a1batross commented Jun 30, 2018

fuzun commented Jun 30, 2018 • edited Loading

fuzun commented Feb 18, 2018 •

edited

Loading

Pull request for the v1: #343
Discussion: https://forums.blurbusters.com/viewtopic.php?f=7&t=3815

a1batross Feb 18, 2018 •

edited

Loading

fuzun Feb 18, 2018 •

edited

Loading

fuzun commented Jun 30, 2018 •

edited

Loading