Texture warping broken since texture refactor

[Marisa the Magician]

Update: I've also found that this affects flats with a warp effect on them too, in very glitchy and, apparently just on windows, crashy ways.

I used Rachael's smooth fluids to make the nukage warp here.

[Rachael]

I've split this since this is a separate report. It's still crashing in Windows.

[drfrag]

Now it only glitches and crashes with the truecolor software renderer (vid_rendermode 1). It's better than before.
It looks bad but only changes color and it's completely broken intermitently. The borders are affected and goes away looking somewhere else or getting closer.

Screenshot_Doom_20190915_103353.png

Screenshot_Doom_20190915_103349.png

With 'r_multithreaded 0' i get a stack trace and several <Unable to read memory> for this in the last step. Wall of text:

Code: Select all

>	gzdoom.exe!swrenderer::DrawSpan32T<swrenderer::DrawSpan32TModes::OpaqueSpan>::Execute(DrawerThread * thread) Line 131	C++
 	[Inline Frame] gzdoom.exe!DrawerCommandQueue::Push(const swrenderer::SpanDrawerArgs &) Line 207	C++
 	gzdoom.exe!swrenderer::SWTruecolorDrawers::DrawSpan(const swrenderer::SpanDrawerArgs & args) Line 215	C++
 	[Inline Frame] gzdoom.exe!swrenderer::SpanDrawerArgs::DrawSpan(swrenderer::RenderThread * thread) Line 133	C++
 	gzdoom.exe!swrenderer::RenderFlatPlane::RenderLine(int y, int x1, int x2) Line 273	C++
 	gzdoom.exe!swrenderer::PlaneRenderer::RenderLines(swrenderer::VisiblePlane * pl) Line 73	C++
 	gzdoom.exe!swrenderer::RenderFlatPlane::Render(swrenderer::VisiblePlane * pl, double _xscale, double _yscale, int alpha, bool additive, bool masked, FDynamicColormap * colormap, FSoftwareTexture * texture) Line 149	C++
 	gzdoom.exe!swrenderer::VisiblePlane::Render(swrenderer::RenderThread * thread, int alpha, bool additive, bool masked) Line 140	C++
 	gzdoom.exe!swrenderer::VisiblePlaneList::Render() Line 346	C++
 	gzdoom.exe!swrenderer::RenderScene::RenderThreadSlice(swrenderer::RenderThread * thread) Line 294	C++
 	gzdoom.exe!swrenderer::RenderScene::RenderThreadSlices() Line 238	C++
 	gzdoom.exe!swrenderer::RenderScene::RenderActorView(AActor * actor, bool renderPlayerSprites, bool dontmaplines) Line 177	C++
 	gzdoom.exe!swrenderer::RenderScene::RenderView(player_t * player, DCanvas * target, void * videobuffer, int bufferpitch) Line 134	C++
 	gzdoom.exe!FSoftwareRenderer::RenderView(player_t * player, DCanvas * target, void * videobuffer, int bufferpitch) Line 201	C++
 	gzdoom.exe!SWSceneDrawer::RenderView(player_t * player) Line 113	C++
 	gzdoom.exe!OpenGLRenderer::FGLRenderer::RenderView(player_t * player) Line 236	C++
 	[External Code]	
 	[Inline Frame] gzdoom.exe!std::_Func_class<void>::operator()() Line 781	C++
 	[Inline Frame] gzdoom.exe!D_Render(std::function<void __cdecl(void)>) Line 380	C++
 	gzdoom.exe!D_Display() Line 781	C++
 	gzdoom.exe!D_DoomLoop() Line 1038	C++
 	gzdoom.exe!D_DoomMain() Line 2731	C++
 	gzdoom.exe!DoMain(HINSTANCE__ * hInstance) Line 992	C++
 	gzdoom.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * nothing, wchar_t * cmdline, int nCmdShow) Line 1324	C++

>	gzdoom.exe!swrenderer::DrawSpan32T<swrenderer::DrawSpan32TModes::OpaqueSpan>::Execute(DrawerThread * thread) Line 131	C++

		Loop	Loop (16)	SoundStream::<unnamed-enum-Mono>
+		shade_constants	{light_alpha=32 light_red=0 light_green=32 ...}	swrenderer::ShadeConstants
+		texdata	{width=32 height=32 xone=134217728 ...}	swrenderer::DrawSpan32T<swrenderer::DrawSpan32TModes::OpaqueSpan>::TextureData
+		this	0xff13230b976be9ce {args={dc_normal={X=??? Y=??? Z=??? } dc_viewpos={X=??? Y=??? Z=??? } dc_viewpos_step=...} }	swrenderer::DrawSpan32T<swrenderer::DrawSpan32TModes::OpaqueSpan> *
+		thread	0x0000023800000000 {thread={_Thr={_Hnd=??? _Id=??? } } current_queue=??? core=??? ...}	DrawerThread *

>	gzdoom.exe!swrenderer::RenderFlatPlane::RenderLine(int y, int x1, int x2) Line 273	C++

+		Thread	0x00000238460a0ac0 {Scene=0x000002384177ee68 {dontmaplines=false clearcolor=0 Threads={ size=1 } ...} ...}	swrenderer::RenderThread *
+		drawerargs	{dc_normal={X=0.000000000 Y=0.000000000 Z=1.00000000 } dc_viewpos={X=145.780975 Y=1053.63391 Z=-111.687149 } ...}	swrenderer::SpanDrawerArgs
		r_modelscene	false	bool

>	gzdoom.exe!swrenderer::PlaneRenderer::RenderLines(swrenderer::VisiblePlane * pl) Line 73	C++

+		spanend	0x000000727f0fbc48 {0, 0, 0, -16324, 10486, -28836, -2622, 16397, -18637, 16008, 1024, 0, 1024, 0, 0, ...}	short[5000]
		t2	436	int
+		this	0x000000727f0fbc40 {Thread=0x00000238460a0ac0 {Scene=0x000002384177ee68 {dontmaplines=false clearcolor=...} ...} ...}	swrenderer::PlaneRenderer * {swrenderer::RenderFlatPlane}

+		this	0x000000727f0fbc40 {Thread=0x00000238460a0ac0 {Scene=0x000002384177ee68 {dontmaplines=false clearcolor=...} ...} ...}	swrenderer::RenderFlatPlane *
		x2	810	int

[drfrag]

Woah! Some "progress", i expected a mighty crash but now i get a more "interesting" effect and it doesn't crash.

Screenshot_Doom_20190918_130212.png

Code: Select all

--- a/src/rendering/swrenderer/textures/warptexture.cpp
+++ b/src/rendering/swrenderer/textures/warptexture.cpp
@@ -60,7 +60,7 @@ const uint32_t *FWarpTexture::GetPixelsBgra()
 	if (time != GenTime[2])
 	{
 		auto otherpix = FSoftwareTexture::GetPixelsBgra();
-		WarpedPixelsRgba.Resize(GetWidth() * GetHeight());
+		WarpedPixelsRgba.Resize(GetWidth() * GetHeight() * 4);
 		WarpBuffer(WarpedPixelsRgba.Data(), otherpix, GetWidth(), GetHeight(), WidthOffsetMultiplier, HeightOffsetMultiplier, time, mTexture->shaderspeed, bWarped);
 		FreeAllSpans();
 		GenTime[2] = time;

[Rachael]

Thank you, drfrag! That gave me some ideas on how to fix this. I am testing the fix now before pushing, but it looks like we've pretty much got this.

[Rachael]

Incomplete work put into a pull request.

This fixes the problem completely in the classic 8-bit renderer, however the truecolor renderer is missing mipmaps and must be completed before this can be accepted. This is unfortunately where I am lost.

[drfrag]

Really? I'm completely lost here. The 8 bit renderer already worked well, AFAIK Carmack 32 bit truecolor is the only one affected.
Even changing that line to 'WarpedPixelsRgba.Resize(GetWidth() * GetHeight() + 1);' prevents the crash.
Edit: i've even tried to fill the buffer with 'WarpBuffer(WarpedPixelsRgba.Data(), otherpix, GetWidth()*2, GetHeight()*2, WidthOffsetMultiplier, HeightOffsetMultiplier, time, mTexture->shaderspeed, bWarped);' and it's better but there are artifacts.

[Rachael]

No, that is very much not the case. Running a test case myself and I got a garbled texture in the 8-bit renderer without the patch I created.

[drfrag]

I see, i've tried only E1M1.

[Rachael]

I don't think you really understand what the individual lines are actually doing.

The texture data is being accessed from a source texture and a new, temporary, software-only texture is created for the warp. If a texture is supposed to be warped, then the renderer will call the texture's "WarpTexture::GetPixels" method, which calls the functions you have modified, depending on whether truecolor mode is active or not.

Code: Select all

		auto otherpix = FSoftwareTexture::GetPixelsBgra();

This stores a reference to the source texture in its original form in a variable called 'otherpix'

Code: Select all

		WarpedPixelsRgba.Resize(GetWidth() * GetHeight());

The name here is a bit of a misnomer. WarpedPixelsRgba is just a TArray which is GZDoom's standard array type. Resizing simply means it is reallocating the buffer. Without this - you get the crashes that were mentioned earlier in this thread. This line in its unmodified form does not account for the larger textures that are created in the resize, hence the crashing issue.

The reason it works when you do "+1" or some other crazy bullshit like that is because a program's data space is allocated in chunks - so there was enough space for the extra garbage even though it technically was not actually allocated. This would be like if a file on a disk used its extra wasted sector space, which even though the sectors are allocated, the individual bytes are not in use.

As you can guess, putting "+ 1" is a hugely improper fix to this solution and you should not do that. Always allocate your arrays to the proper size that you actually need!

Code: Select all

		WarpBuffer(WarpedPixelsRgba.Data(), otherpix, GetWidth(), GetHeight(), WidthOffsetMultiplier, HeightOffsetMultiplier, time, mTexture->shaderspeed, bWarped);

This executes the actual texture warp on the new texture being created, using the 'otherpix' texture that was referenced earlier. In the patch this is fixed so that GetWidth() and GetHeight() are set to the proper size, that way the entire texture is warped, and is warped in the proper (resized) dimensions.

Code: Select all

		FreeAllSpans();

I'm not 100% sure about this, but I would reasonably guess that all this does is unallocate some now unused texture spans.

Code: Select all

		GenTime[2] = time;

This simply sets the check at the start of this code block so that it doesn't execute until the next frame. This is important because you only want to generate a warp texture once per frame; not once per pixel.

[Rachael]

https://github.com/madame-rachelle/gzdo ... ee91fc2771

Commit with mipmaps fix (I think?). It should be in the pull request already, too.

If this looks good, I'll go ahead and merge it.

[drfrag]

Thanks for the explanation. I mostly understood what the code does, i had no idea about the Gentime tough.
The *4 was a blind shot, i thought that a pixel in truecolor takes 4 times the space as in paletted. Then i wanted to see if it crashed with less and i put +1 (i think that actually doubles the size) but it was not a fix. I was surprised that Softpoly apparently worked well. Also i thought that HQ resize modes were disabled by default, i never would have thought about that.
Edit: i see that the new FWarpTexture::GenerateBgraMipmapsFast() is identical to FSoftwareTexture::GenerateBgraMipmapsFast(), can't you just use that one?

[Rachael]

No, it is not an exact duplicate, and the reason the code is duplicated is because it accesses a member variable of FTextureWarp that is not available in FSoftwareTexture. Both functions should probably be rewritten as one with a variable pointer parameter instead, but that's something someone can worry about later. (if they care enough and they have enough time)

[drfrag]

Now i've noticed, at first glance they looked identical.

[drfrag]

Hey i still don't see it, gl_texture_hqresizemult by default is 1 and why the *2 for the Resize?