CET with AMD and my camera tools doing viewport resizing causes crashes

[FransBouma]

Hi,

I'm the developer of the cyberpunk camera tools used by many people. One of the features is to resize the game's viewport by setting the new resolution in memory, after which the engine will resize the window and viewport to the new resolution. This is called 'Hotsampling' and is often used by virtual photographers to take a screenshot at a higher resolution.

Resizing the viewport of the game that way when CET is used however leads to a specific crash in ffx_backend_dx12_x64.dll:

When CET isn't active, no crash occurs.

Looking at your code, it seems you're hooking the engine's resize function instead of the swapchain's ResizeBuffers (d3d12 swapchain VTable[13]).

CyberEngineTweaks/src/d3d12/D3D12_Hooks.cpp

Line 269 in 4aea400

if (MH_CreateHook(resizeInternal.GetAddr(), reinterpret_cast<void*>(&CRenderGlobal_Resize), reinterpret_cast<void**>(&m_realInternalResize)) != MH_OK ||

My question is: why not hook the swapchain resize buffers to get the callback, then do the cleanup? Could this be changed perhaps in the future so these crashes on AMD cards are avoided?

The resolution struct is interceptible using AOB: 8B 41 7C 39 41 18 75 19 8B 81 80 00 00 00 39 41 1C 75 0E, where rcx contains the structure pointer. offset 0x7C in that struct contains the screen width, offset 0x80 contains the height (in pixels). Changing any of the values there will resize the viewport by the engine and will then trigger the crash on AMD hardware.

Thanks!

[WSSDude]

This is effecfively duplicate of #911 . It would be better if this was fixed inside of ImGui as the issue states and there is MR that just keeps getting rebased and not merged...

[FransBouma]

There's no issue inside ImGui to fix, as my own code, using ImGui works fine in Cyberpunk, when hooking the swapchain's resize buffers. Closing this won't make it go away magically by hoping ImGui 'might fix' things, as there's nothing wrong with ImGui as the issue isn't even related to ImGui. I reported it so the CET devs are aware of this and as there's an alternative to what they're doing (hooking a different function), things would still work as they do today and also don't crash when other tools are used.

The issue you're linking to is caused by the same underlying problem, which is not ImGui related. ImGui has nothing to do with it.

[WSSDude]

Why not make a MR instead then? 😄

I know it can be fixed by manually doing font atlas rebuild etc. but we discussed this years back and there is already a MR for ImGui to have proper font atlas updating without rebuilding accounting for all of this.

I consider this a workaround at best.

[WSSDude]

And I did not close original issue, did I now? 😉

[WSSDude]

Also, it is not as simple as rebuilding the font atlas, there should be multiple textures in place to account for frames in flight etc. It would need much more changes to the backends than you wrote.

[FransBouma]

For some reason I can't get CET to compile on my system. (I use visual studio, not clang).
Anyway, you already have all the reset code in place. My suggestion is merely about instead of calling that code from the hook into the engine's resize function, you hook the swapchain's resizebuffers function and then call it from there.

Font atlas building isn't related to this. It's purely the timing where to call the d3d12 reset: instead of hooking the engine's resize function, hook the swapchain's resizebuffers function.

CET is your repo, you can of course close this for whatever reason, and open source takes time / energy and there are perhaps more pressing matters to attend energy to. What I'm a bit surprised by is that this is closed by pointing to an issue that IMHO has no relevance to this one, effectively making this issue be ignored while it shouldn't be. But alas, at the end of the day we can only do so much so if this is low priority (which I can imagine it is btw, no worries there :) ), then just close it because of that.

[WSSDude]

Look, backends are 1:1 nearly with ImGui ones.

Reset functions is actually broke itself due to frames in flight issue as mentioned, ImGui backend do not count with them at all, it needs rewrite and not small.

If it makes you happier, Ill close the issues other way around but truth is this is an ImGui issue atm because of their backend being too simple for their own good.

Only CET issue here is us using their backends. It is easier to maintain and keep somewhat feature parity this way.

As said, if you want to rewrite it, Im not blocking you from it. But you have to consider this is really not just about calling some Reset function, there are multiple underlying issues causing all of this that such simple fix would still not make work as you would wish. Already tried that, and had to revert it because it made matters worse in some regards.

[WSSDude]

I finally found the MR I had in mind, I'll link it here.

ocornut/imgui#3761

There should be something like this first and foremost.

As you can see, there needs to be also some proper barrier for the font texture, not just rebuild.

This should not really affect our AddTexture as that is not expected to get resized so no barrier should be needed there.

[FransBouma]

Hmm. What's interesting is that I use the default font and atlas for ImGui and have no problems whatsoever when the buffers are resized, but I guess you use custom fonts and therefore get this problem... and resizing the buffers causes your code to reupload the textures, causing the issue.

What's odd tho is that on NVidia cards, it all works fine. Only on AMD cards it fails. It also worked ok till CP patch 2.13 btw.

Reshade also supports custom fonts and doesn't have this problem either btw, but not sure if Crosire has merged the PR in imgui you link to into his custom ImGui build... I now see what you mean with rebasing: The branch gets rebased over and over but not merged...

[WSSDude]

Unsure about your code, but I believe ReShade have their own backends at least, that may answer that part.

When it comes to Nvidia vs. AMD, AMD drivers have much less checks and safety guards which makes them faster in some regard but they also like to crash more from experience.

But even Nvidia has issue with this from what we found throughout the years so this in particular is probably more related to the raw power of the GPU in question I suppose. As even Nvidia driver may cause a crash due to this if the GPU is lower-end.

I suppose fast enough GPU makes the frames in flight issue sort of go away, as there are not really any probably when the font rebuild happens. Is just a theory though.

[WSSDude]

One more thing that may make this more apparent on CET that came to mind now is that we tripple-buffer internally due to users nonstop using onDraw incorrectly, which forced us to take a bit extreme safety measure I suppose and basically render everything on main thread right after onUpdate is called.

In the render thread before present, we show last successfully rendered ImGui frame from the render data it made, effectively lagging behind a bit.

[WSSDude]

And it also just came to mind that, due to the triple-buffer thingy, even if this was fixed in ImGui or when we copy the MR somewhat, we would probably also want to reset the buffers during the resize as it would reference the old texture. Unsure how that would behave/look.

It may render glyphs wrongly for at most 2 frames (probably no biggy) or it may crash as the texture was changed, unsure what would exactly happen but that is also something that would need to be taken care of in any case when this should be fixed.

[WSSDude]

Actually no, I accounted for that already it seems, that should happen when Reset is called already.

[FransBouma]

Hmm, ok, so you're not rendering on the present callback (so dxgi present is called by the game, you render your stuff on the command queue of the engine, and then call present) ? I think that's the crucial difference between my (and reshade) code and yours. Tho as I draw an overlay and you likely do much more than that it might not be comparable (as in: CET might need to render stuff before the game renders e.g. the UI, no idea)

[WSSDude]

Hmm, ok, so you're not rendering on the present callback (so dxgi present is called by the game, you render your stuff on the command queue of the engine, and then call present)

We are hooked to game present function which calls DXGI present. That function does not do anything else, is very small when you look at disassembly. We do our stuff on the game command queue, and then let the game do the DXGI present call as it would normally.

Just to clarify but I believe we mean the same thing essentially.

And all we do is render already prepared ImDrawLists we get from main thread. The real rendering happens still ofc on the render thread before the DXGI present happens. But we use precomputed data at that point, we do not compute it on the render thread.

So we do render on that thread, probably wrote it wrongly... We just use pre-computed data, wrote it a bit weirdly before. Believe ImGui calls that step Render even though all it does is make an ImDrawList batch that needs to be actually rendered.

[WSSDude]

We do it like this primarilly for the threading issues sort of as mentioned, render thread runs in parallel to main and when we had onDraw and whole ImGui::Render happen on the render thread, we were nonstop getting complains about different types of crashes and race conditions happening.

We ended up doing everything on the main thread after game updates everything. Only thread doing real work at that point are streaming-related and rendering related, so it is a safe space of sorts to call into game with custom scripts (nothing should cause race conditions + a lot of things can only be executed on main thread, otherwise it crashes with assertion error about incorrect thread being used).

Everything would be much simpler if users would not be trying to call into the game itself or otherwise access game resources there as was originally intended, but one can only dream I guess. onDraw was meant only for ImGui and their own local stuff, but nobody used it right probably ever and here we ended.

It is definitely slower overall and I suppose makes these sort of issues more apparent, but it was only safe way to handle this reliably without breaking ton of existing mods.

[FransBouma]

Got it. Yeah ImGui just creates draw calls, the actual rendering is done when you handle present indeed, so frames in flight shouldn't be a problem, but not sure what mods are doing during the frame, so I can understand your concern/the way it's done now...