View Issue Details

IDProjectCategoryView StatusLast Update
0006434The Dark ModGraphicspublic14.04.2024 02:07
Reporterstgatilov Assigned Tostgatilov  
PrioritynormalSeverityfeatureReproducibilitysometimes
Status feedbackResolutionopen 
Product VersionTDM 2.12 
Target VersionTDM 2.13 
Summary0006434: Try to fully support nested/recursive subviews
DescriptionI have reviewed the subviews code, and it seems that there is no inherent problem in having arbitrary nested subviews.

I mean the cases like:
1) Remote camera looking at mirror, or mirror reflecting remote screen.
2) Remote camera + mirror cases combined with skybox.
3) Several mirrors on the same scene.
4) Several mirrors showing reflection of each other, possible infinitely (should be capped be engine).

Indeed, such effects would cost a lot of performance.
But with hard limits like we have now, mappers can fall into one of these cases accidentally and get broken rendering.
Steps To ReproduceTODO: Need an ambitious test map for this.
TagsNo tags attached.

Relationships

related to 0006424 resolvedstgatilov Remote camera rendering gives warning and breaks sky 
related to 0003108 resolvedgrayman Merge 7318's 3D Skybox Code 

Activities

stgatilov

stgatilov

06.01.2024 16:36

administrator   ~0016326

After looking at 0006424, it seems that the only problem right now is using texture of hardcoded name as output for mirror / remote subviews.

Instead, we can leave output texture name undefined in the material file (e.g. "$auto$").
Then the name of the texture should be generated from recursion path of subviews to keep them distinct.
Note that several mirrors in the same scene should get different names too.
The challenge is to ensure that this auto-generated name is used in this material in the parent view.

Note that there are nasty exceptions.
For instance, skybox does not copy output to texture, but simply leaves itself as background.
This, there can be only one skybox and it must be rendered last (among all subviews of the same view).
stgatilov

stgatilov

08.01.2024 14:54

administrator   ~0016347

Added test/6434_nested_subviews.map for testing.
stgatilov

stgatilov

08.01.2024 15:05

administrator   ~0016348

Last edited: 08.01.2024 15:13

Some WIP attached.

I created a storage of "subview output images" in RenderSystem.
Whenever subview is generated, an image is taken out of this storage of matching size (there there is none, then a new one is generated).
The copy command is done into this image, and then it is attached to drawSurf_t to override material's texture (works only inside "simple" surface pass, just like e.g. cinematics).
If some image in the storage is not used for 100 frames, then it is garbage-collected.

There are various blockers for recursing into mirrors, for recursing into same surface, etc.
I only tested on remote renders and mirrors, so I deleted all blockers from there.
There is still a sanity blocker by recursion depth (output texture is _black if blocked), and maybe we should add more sanity blockers for performance reasons.

I noticed some issues with mirror.
Right now it sets a clip plane, otherwise objects slightly behind mirror will mess up everything.
But this clip plane is only taken into account in depth stage, although I see problems at least with stencil shadows too.
Besides, I think we need several clip planes if we want to support mirror inside mirror.

The skybox is broken inside both remote renders and mirrors.
I think its logic simple does not support any kind of subviews --- should be fixed there.

Finally, I did not test X-ray.
Right now it is handled separately for some reason despite it copying the output into texture.
Perhaps I need to refactor it to works similar to remote renders.

P.S. And thief looks black in all subviews, which is also perhaps a bug.
P.P.S. Restoring a possibility to render mirrored view in lower resolution would be nice for performance too.
6434_nested_subviews_wip.patch (10,510 bytes)   
Index: renderer/backend/stages/SurfacePassesStage.cpp
===================================================================
--- renderer/backend/stages/SurfacePassesStage.cpp	(revision 10623)
+++ renderer/backend/stages/SurfacePassesStage.cpp	(working copy)
@@ -334,7 +334,7 @@
 
 	// bind texture (either 2D texture or cubemap is used)
 	GL_SelectTexture( 0 );
-	BindVariableStageImage( &pStage->texture, regs );
+	BindVariableStageImage( drawSurf, &pStage->texture, regs );
 	if ( texgen == TEXGEN_CUBEMAP ) {
 		uniforms->cubemap.Set( 0 );
 		uniforms->texture.Set( 1 );
@@ -549,7 +549,7 @@
 	RB_DrawElementsWithCounters( drawSurf, DCK_SURFACE_PASS );
 }
 
-void SurfacePassesStage::BindVariableStageImage( const textureStage_t *texture, const float *regs ) {
+void SurfacePassesStage::BindVariableStageImage( const drawSurf_t *drawSurf, const textureStage_t *texture, const float *regs ) {
 	if ( texture->cinematic ) {
 		if ( r_skipDynamicTextures.GetBool() ) {
 			globalImages->defaultImage->Bind();
@@ -568,5 +568,8 @@
 		}
 	} else if ( texture->image ) {
 		texture->image->Bind();
+	} else if ( texture->dynamic && drawSurf->dynamicImageOverride ) {
+		// stgatilov #6434: image generated by subview
+		drawSurf->dynamicImageOverride->Bind();
 	}
 }
Index: renderer/backend/stages/SurfacePassesStage.h
===================================================================
--- renderer/backend/stages/SurfacePassesStage.h	(revision 10623)
+++ renderer/backend/stages/SurfacePassesStage.h	(working copy)
@@ -54,5 +54,5 @@
 	void DrawSoftParticle( const drawSurf_t *drawSurf, const shaderStage_t *pStage );
 	void DrawCustomShader( const drawSurf_t *drawSurf, const shaderStage_t *pStage );
 
-	void BindVariableStageImage( const textureStage_t *texture, const float *regs );
+	void BindVariableStageImage( const drawSurf_t *drawSurf, const textureStage_t *texture, const float *regs );
 };
Index: renderer/frontend/tr_light.cpp
===================================================================
--- renderer/frontend/tr_light.cpp	(revision 10623)
+++ renderer/frontend/tr_light.cpp	(working copy)
@@ -1269,6 +1269,7 @@
 	drawSurf->CopyGeo( tri );
 	drawSurf->space = space;
 	drawSurf->material = material;
+	drawSurf->dynamicImageOverride = nullptr;
 	drawSurf->scissorRect = scissor;
 	drawSurf->sort = material->GetSort();
 	drawSurf->dsFlags = 0;
Index: renderer/frontend/tr_subview.cpp
===================================================================
--- renderer/frontend/tr_subview.cpp	(revision 10623)
+++ renderer/frontend/tr_subview.cpp	(working copy)
@@ -19,12 +19,14 @@
 #include "renderer/tr_local.h"
 #include "game/Grabber.h"
 
+
+static const int MAX_SUBVIEW_DEPTH = 4;
+
 typedef struct {
 	idVec3		origin;
 	idMat3		axis;
 } orientation_t;
 
-
 /*
 =================
 R_MirrorPoint
@@ -274,9 +276,9 @@
 */
 static void R_RemoteRender( drawSurf_t *surf, textureStage_t *stage ) {
 
-	// remote views can be reused in a single frame
+/*	// remote views can be reused in a single frame
 	if ( stage->dynamicFrameCount == tr.frameCount ) 
-		return;
+		return;*/
 
 	// if the entity doesn't have a remoteRenderView, do nothing
 	if ( !surf->space->entityDef->parms.remoteRenderView ) 
@@ -288,6 +290,8 @@
 
 	parms->isSubview = true;
 	parms->isMirror = false;
+	// if we see remote screen in mirror, drop mirror's clip plane
+	parms->clipPlane = nullptr;
 
 	parms->renderView = *surf->space->entityDef->parms.remoteRenderView;
 	parms->renderView.viewID = VID_SUBVIEW;	// clear to allow player bodies to show up, and suppress view weapons
@@ -314,11 +318,17 @@
 	R_RenderView(*parms);
 
 	// copy this rendering to the image
-	stage->dynamicFrameCount = tr.frameCount;
+	/*stage->dynamicFrameCount = tr.frameCount;
 	if ( !stage->image )
 		stage->image = globalImages->scratchImage;
 
-	tr.CaptureRenderToImage( *stage->image->AsScratch() );
+	tr.CaptureRenderToImage( *stage->image->AsScratch() );*/
+
+	stage->image = nullptr;
+	idImageScratch *outputTexture = tr.CreateImageForSubview();
+	tr.CaptureRenderToImage( *outputTexture );
+	surf->dynamicImageOverride = outputTexture;
+
 	tr.UnCrop();
 }
 
@@ -330,13 +340,13 @@
 void R_MirrorRender( drawSurf_t *surf, textureStage_t *stage, idScreenRect& scissor ) {
 	viewDef_t		*parms;
 
-	if ( tr.viewDef->superView && tr.viewDef->superView->isSubview ) // #4615 HOM effect - only draw mirrors from player's view and top-level subviews
+	/*if ( tr.viewDef->superView && tr.viewDef->superView->isSubview ) // #4615 HOM effect - only draw mirrors from player's view and top-level subviews
 		return;
 
 	// remote views can be reused in a single frame
 	if ( stage->dynamicFrameCount == tr.frameCount ) {
 		return;
-	}
+	}*/
 
 	// issue a new view command
 	parms = R_MirrorViewBySurface( surf );
@@ -365,11 +375,17 @@
 	R_RenderView( *parms );
 
 	// copy this rendering to the image
-	stage->dynamicFrameCount = tr.frameCount;
+	/*stage->dynamicFrameCount = tr.frameCount;
 	if ( !stage->image )
 		stage->image = globalImages->scratchImage;
 
-	tr.CaptureRenderToImage( *stage->image->AsScratch() );
+	tr.CaptureRenderToImage( *stage->image->AsScratch() );*/
+
+	stage->image = nullptr;
+	idImageScratch *outputTexture = tr.CreateImageForSubview();
+	tr.CaptureRenderToImage( *outputTexture );
+	surf->dynamicImageOverride = outputTexture;
+
 	//tr.UnCrop();
 }
 
@@ -678,7 +694,6 @@
 */
 bool	R_GenerateSurfaceSubview( drawSurf_t *drawSurf ) {
 	idBounds		ndcBounds;
-	viewDef_t		*parms;
 	const idMaterial		*shader;
 
 	// for testing the performance hit
@@ -690,7 +705,7 @@
 
 	shader = drawSurf->material;
 
-	// never recurse through a subview surface that we are
+	/*// never recurse through a subview surface that we are
 	// already seeing through
 	for ( parms = tr.viewDef ; parms ; parms = parms->superView ) {
 		if ( parms->subviewSurface
@@ -700,6 +715,12 @@
 		}
 	}
 	if ( parms ) 
+		return false;*/
+
+	int depth = 0;
+	for (viewDef_s *view = tr.viewDef; view; view = view->superView)
+		depth++;
+	if (depth > MAX_SUBVIEW_DEPTH)
 		return false;
 
 	// crop the scissor bounds based on the precise cull
@@ -787,10 +808,14 @@
 		if ( !shader || !shader->HasSubview() )
 			continue;
 
-		if ( shader->GetSort() != SS_PORTAL_SKY ) // portal sky needs to be the last one, and only once
+		if ( shader->GetSort() != SS_PORTAL_SKY ) {// portal sky needs to be the last one, and only once
 			if ( R_GenerateSurfaceSubview( drawSurf ) ) {
 				subviews = true;
+			} else {
+				// #6434: probably blocked due to limits: render as black image
+				drawSurf->dynamicImageOverride = globalImages->blackImage;
 			}
+		}
 	}
 
 	static bool dontReenter = false;
Index: renderer/RenderSystem.cpp
===================================================================
--- renderer/RenderSystem.cpp	(revision 10623)
+++ renderer/RenderSystem.cpp	(working copy)
@@ -702,6 +702,8 @@
 	// we can now release the vertexes used this frame
 	vertexCache.EndFrame();
 
+	FreeOldSubviewImages();
+
 	if ( session->writeDemo ) {
 		session->writeDemo->WriteInt( DS_RENDER );
 		session->writeDemo->WriteInt( DC_END_FRAME );
@@ -741,6 +743,70 @@
 	viewport.y2 = idMath::FtoiTrunc( ( rc.y + rc.height ) - floor( renderView.y * hRatio + 0.5f ) - 1 );
 }
 
+
+void idRenderSystemLocal::FreeOldSubviewImages() {
+	// this method should be called outside parallel section
+	assert(!session->IsFrontend());
+
+	static const int SUBVIEW_IMAGE_KILL_AFTER_FRAMES = 100;
+
+	for (int i = 0; i < subviewImages.Num(); i++) {
+		ImageForSubview &si = subviewImages[i];
+		if (si.purged)
+			continue;
+
+		if (frameCount - si.lastUsedFrameCount > SUBVIEW_IMAGE_KILL_AFTER_FRAMES) {
+			si.purged = true;
+			si.image->PurgeImage();
+		}
+	}
+}
+
+idImageScratch *idRenderSystemLocal::CreateImageForSubview() {
+	// should be called from frontend
+	assert(!com_smp.GetBool() || session->IsFrontend());
+
+	renderCrop_t &rc = renderCrops[currentRenderCrop];
+	int width = rc.width;
+	int height = rc.height;
+
+	int slot = -1;
+	for (int i = 0; i < subviewImages.Num(); i++) {
+		ImageForSubview &si = subviewImages[i];
+
+		if (si.purged) {
+			slot = i;
+			continue;	// GPU texture was freed
+		}
+
+		if (si.lastUsedFrameCount == frameCount) {
+			continue;	// already occupied for this frame
+		}
+
+		if (si.width == width && si.height == height) {
+			// can reuse texture (not freed yet)
+			si.lastUsedFrameCount = frameCount;
+			return si.image;
+		}
+	}
+
+	if (slot < 0) {
+		// add new texture to the end
+		ImageForSubview &added = subviewImages.Alloc();
+		slot = subviewImages.IndexOf(&added);
+		added.width = width;
+		added.height = height;
+
+		idStr imageName = va("$subview$%d", slot);
+		added.image = globalImages->ImageScratch(imageName);
+	}
+
+	ImageForSubview &si = subviewImages[slot];
+	si.purged = false;	// will be generated in backend when data is copied into it
+	si.lastUsedFrameCount = frameCount;
+	return si.image;
+}
+
 static int RoundDownToPowerOfTwo( int v ) {
 	int	i;
 
Index: renderer/tr_local.h
===================================================================
--- renderer/tr_local.h	(revision 10623)
+++ renderer/tr_local.h	(working copy)
@@ -153,6 +153,7 @@
 
 	const struct viewEntity_s *space;
 	const idMaterial		*material;			// may be NULL for shadow volumes
+	idImage					*dynamicImageOverride;	// stgatilov: if not NULL, then texture of material should be replaced with this one (for subviews)
 	float					sort;				// material->sort, modified by gui / entity sort offsets
 	const float				*shaderRegisters;	// evaluated and adjusted for referenceShaders
 	/*const*/ struct drawSurf_s	*nextOnLight;	// viewLight chains
@@ -819,6 +820,13 @@
 
 static const int	MAX_RENDER_CROPS = 8;
 
+struct ImageForSubview {
+	bool purged = false;
+	int width = -1, height = -1;
+	idImageScratch *image = nullptr;
+	int lastUsedFrameCount = INT_MIN;
+};
+
 /*
 ** Most renderer globals are defined here.
 ** backend functions should never modify any of these fields,
@@ -877,6 +885,9 @@
 	void					Clear( void );
 	void					RenderViewToViewport( const renderView_t &renderView, idScreenRect &viewport );
 
+	idImageScratch *		CreateImageForSubview();
+	void					FreeOldSubviewImages();
+
 public:
 	// renderer globals
 	bool					takingScreenshot;
@@ -931,6 +942,8 @@
 
 	unsigned short			gammaTable[256];	// brightness / gamma modify this
 	idParallelJobList*		frontEndJobList;
+
+	idList<ImageForSubview> subviewImages;
 };
 
 extern backEndState_t		backEnd;
6434_nested_subviews_wip.patch (10,510 bytes)   
stgatilov

stgatilov

04.04.2024 20:37

administrator   ~0016617

Thief looking black in the mirror is not a bug.

Thief looks normal in AC2 and on another test map.
However, I noticed that no light sources interact with player model: it is only lit by ambient light.
The test map used here had no ambient light.
As soon as I added it, the thief was no longer black.
stgatilov

stgatilov

06.04.2024 09:19

administrator   ~0016619

Speaking of the sky.

First, there are two techniques of settings up where sky is.
One is to fill spaces with portalsky, another one is to leave caulk so that skybox implicitly remains in framebuffer and is not overdrawn with anything else.
I'm not sure yet whether this choice affects what happens here.

Another variation is the "type" spawnarg on "info_portalsky" entity:
  0 "default": viewpoint is always at the origin of portalsky, thus skybox does not move relative to viewer
  1 "global": viewpoint moves along with player with some "scale" multiplier, so items in skybox scene move relative to the viewer
  2 "local" same as global, but I think it does not move when it is not active
All three types are present in map/skiestdm.map.
There are two missions which use 1 "global": Williame Steele 2 and 3. All the other usages stick to 0 "default".

The default mode only allows to render infinitely far objects.
Non-default modes allow to render objects at certain distance which are certainly behind normal scene.
I suppose it is useful when you want to make some background geometry too large for a normal level.
The local mode seems useless to me because it you exit sky zone at point A then reenter it at point B and go back to point A, the sky will look different?
I'd say mapper needs functional dependence of sky viewport on player viewport.

So, this variation of sky makes it a difficult to support sky in subviews.
I think I'll concentrate on default skybox and portalsky, and include other cases only if it's easy.
stgatilov

stgatilov

11.04.2024 19:52

administrator   ~0016635

Lots of changes committed:
  r10701 Fixed skybox in mirrors.
  r10702 It is now possible to have recursively nested subviews (although inefficient).

The story of skyboxes ended pretty easily.
The default mode sets camera origin to fixed point, so it works equally fine if skybox is nested into some other subview.
There was minor issue with mirroring: portal subview did not inherit properties of the parent subview, now they do.

Several clip planes for nested mirrors are not necessary.
The most-nested clip plane cuts all view rays later than any parent clip plane, so only most-nested clip plane is set now.

X-ray also support now.
The most annoying feature of X-ray is that it can be generated from GUI overlay.
This code is refactored now a bit.

The ugliest thing is using subview output in custom shaders (e.g. heatHaze).
Unlike simple texture stages, custom shader stages accept many textures as inputs, and it is not clear which of them should be overriden =(
nbohr1more

nbohr1more

13.04.2024 03:52

developer   ~0016640

Latest SVN build seems to have broken _scratch image behavior and portalsky behavior in the grass demo map.
nbohr1more

nbohr1more

13.04.2024 03:52

developer   ~0016641

stgatilov

stgatilov

13.04.2024 08:17

administrator   ~0016642

Where can I get this grass demo?
nbohr1more

nbohr1more

13.04.2024 12:32

developer   ~0016643

https://www.moddb.com/mods/the-dark-mod/addons/arcturuss-animated-grass-demo
stgatilov

stgatilov

13.04.2024 14:07

administrator   ~0016646

Fixed usage of mirrorRenderMap (or other subview generators) with custom shaders:
  r10711 Fixed custom shaders which use dynamic textures (not only X-ray case).

The demo is not completely broken, but I'm not sure it works as intended.
There are two surfaces with "arcturus_pool" material.
One successfully uses the mirrored image, the other one does not (and uses black image instead).
The reason why that surface does NOT generate a subview is that some of its triangles are backfacing.
The engine only supports planar mirror surfaces: non-planar ones result in pretty undefined behavior.
stgatilov

stgatilov

13.04.2024 14:26

administrator   ~0016647

In general, I think at some moment we'll have to do some cleanup of materials with dynamic textures.
Here is the list of dynamic textures we have now.

::: _cinematic
Filled via UploadScratch from video file at the moment of rendering.
Only used internally, no external references are allowed.

::: _guiRender, _shadowDepthFbo, _shadowAtlas, _currentStencilFbo
Only filled and used internally as framebuffer attachments, no material may reference them.

::: _currentRender, _currentDepth
Filled internally as framebuffer attachments, but exposed to postprocessing materials.
Materials often reference them in order to implement effects like heatHaze, and thus they are public.
Their meaning/behavior is fixed and should not change over time.

::: _scratch
This image has mixed purpose.
On one hand, it is sometimes used in hardcoded ways (screen wipe, doublevision, etc.).
In these cases it is filled from C++ code in special way, and may only be used by a few core materials --- i.e. is completely private.
On the other hand, it is often used as destination of subview-generated images, including user-defined materials (see below).
In this case it becomes rather public and is referenced by materials

::: _dynamic, _scratch2, _xray, _cameraN (and partially _scratch)
These images are subview-generated and are referenced in materials, including user ones.
After this ticket, all these textures never actually exist and are never filled with data, they are merely placeholders in material text to mark that subview-generated image should be used instead.
1) If you have a custom shader stage (the one like heatHaze) with dynamic texture generation (e.g. mirrorRenderMap),
  then each reference to any of these texture is replaced with the texture generated by the subview.
2) If you have "simple texture" / "old" stage with dynamic texture generation, then subview-generated texture is used REGARDLESS of which texture you specify.
  This is because e.g. "textures/common/mirror" material references non-existent "textures/common/mirror" texture.
  Perhaps we should fix these materials and make rules the same.
nbohr1more

nbohr1more

13.04.2024 14:56

developer   ~0016648

Thanks for the water fix!

The sky is still rendering black and this is happening for other missions as well ( For example: "Mission 1: A New Job" , "Written in Stone" )
I assume portalsky behavior is still WIP ?
stgatilov

stgatilov

13.04.2024 16:42

administrator   ~0016649

Fixed here:
  r10712 Restored portalsky subview generation.

In fact, I made sure skybox works fine even in nested mirrors, but accidentally deleted portalsky subview generation later when I worked on X-ray.
nbohr1more

nbohr1more

13.04.2024 18:42

developer   ~0016650

Last edited: 14.04.2024 02:07

Thank you! Everything is functional again!

I have mixed feelings about the whole "scratch" concept.
The fact that it is de-facto volatile makes me wish that we had multiple _currentRender images (etc)
so we can define where we capture the image in comparison to scene completion.
I guess "sort flags" hand that, but it might be better if it were implicit in the image?

Issue History

Date Modified Username Field Change
06.01.2024 16:31 stgatilov New Issue
06.01.2024 16:31 stgatilov Status new => assigned
06.01.2024 16:31 stgatilov Assigned To => stgatilov
06.01.2024 16:31 stgatilov Relationship added related to 0006424
06.01.2024 16:36 stgatilov Note Added: 0016326
08.01.2024 14:54 stgatilov Note Added: 0016347
08.01.2024 15:05 stgatilov Note Added: 0016348
08.01.2024 15:05 stgatilov File Added: 6434_nested_subviews_wip.patch
08.01.2024 15:07 stgatilov Note Edited: 0016348
08.01.2024 15:13 stgatilov Note Edited: 0016348
04.04.2024 20:37 stgatilov Note Added: 0016617
06.04.2024 08:50 stgatilov Relationship added related to 0003108
06.04.2024 09:19 stgatilov Note Added: 0016619
11.04.2024 19:52 stgatilov Note Added: 0016635
11.04.2024 19:52 stgatilov Status assigned => feedback
13.04.2024 03:52 nbohr1more Note Added: 0016640
13.04.2024 03:52 nbohr1more Note Added: 0016641
13.04.2024 03:52 nbohr1more File Added: grass (2024-04-12 23-51-07) (240.73 -819.45 -337.12).jpg
13.04.2024 08:17 stgatilov Note Added: 0016642
13.04.2024 12:32 nbohr1more Note Added: 0016643
13.04.2024 14:07 stgatilov Note Added: 0016646
13.04.2024 14:26 stgatilov Note Added: 0016647
13.04.2024 14:56 nbohr1more Note Added: 0016648
13.04.2024 16:42 stgatilov Note Added: 0016649
13.04.2024 18:42 nbohr1more Note Added: 0016650
14.04.2024 02:07 nbohr1more Note Edited: 0016650