• Fixes FSR2 Crash and Error FFX_ERROR_INVALID_ARGUMENT #105509

Edit - Fix without the resizing is here:

• Stabilize viewport behavior under FSR2 (fallback to Bilinear) #110119 if that is preferred (see comments). The alternative without the resizing does not have concerns for backporting, unlike this PR, in my opinion.

Below is the original message

When a subviewport using FSR2 upscaling falls below 64px on at least one dimension it produces this error spam:

ERROR: Condition "p_job.uavMip[i] >= mip_slice_rids.size()" is true.
Returning: FFX_ERROR_INVALID_ARGUMENT
at: execute_gpu_job_compute_rd
(servers\rendering\renderer_rd\effects\fsr2.cpp:418)

Additionally, it was possible through the scaling factor to have either dimension fall below 2px, crashing the editor.

While testing using the MRP from the linked issue, I found that it was also possible to set the dimensions of the viewport above 16384, which does not crash the editor but causes updating of the subviewport to stop (at least when rendering to a viewport texture) until the editor is restarted (or presumably until rendering of the subviewport is restarted somehow). It seems to me that the code intended to clamp sizes within a valid range but the enforcing was inconsistent, possibly because it assumes the viewport provided to _configure_3d_render_buffers() has a valid size, as the original code makes much more sense with that assumption.

Also, when the code falls back to Bilinear from FSR2, the scaling type is not re-evaluated, leading to it rendering with Bilinear scaling but still using jitter, which causes a lot of wobble, especially on smaller subviewport sizes.

This PR:

• Enforces an upper limit of 16384 to subviewport dimensions

  • This upper limit is hardcoded in various parts of the rendering code and is a sensible value, considering most modern devices support this upper limit and few support higher, so I just added it as a raw value where relevant. I feel it is used so much it should be defined by the preprocessor but it's not a big deal at the moment. This fixes crashing the render thread by exceeding maximum texture limits through a scale factor above 1.0.

• Restores the proper scaling mode type after fallback to Bilinear. This fixes jitter that can occur due to fallback to Bilinear.

• Ensures that the render size of a subviewport that uses FSR2 will always be of valid dimensions, preserving aspect ratio and also ensuring the target size is always equal to or larger than the render size, to preserve a scaling of 1.0 or less. This fixes the error spam and editor crash referenced in the linked issue.

  • This is done by first scaling up the render size so the smaller dimension is at least 2px, then scaling it up again so the larger dimension is at least 64px. The scaling is done on both dimensions proportionally on each pass so as to preserve the aspect ratio and prevent stretching. The scaling up is the minimal possible. The target size is then put through the same process to ensure it is always equal to or larger than the render size on both dimensions (so we're never accidentally downscaling, since this is not supposed to be done with FSR2).

    • This is a spreadsheet that repeats the same calculations for various viewport size dimensions, to test for edge cases and overall behavior. It also tests what would happen if we try to preserve the 3d scaling: https://docs.google.com/spreadsheets/d/1xiGvhqp6tYgkyyENmOtAHOpoSXYu-0OmtalhVGiSE14/edit?usp=sharing

  • Scaling factor is not preserved due to this adjustment but I feel that the intent of the user to use FSR2 is to improve performance and render to a lower resolution, so it makes more sense to preserve the aspect ratio while also keeping it as small as possible, rather than trying to preserve the user-defined render scale. Trying to preserve the original scaling factor can result in the target size growing enormously, which defeats the purpose of using FSR2 on any scale other than 1.0, and at a scaling of 1.0 all we need to do is make sure the dimensions are valid for the subviewport, which is already guaranteed due to the previous changes.

Possible concerns:

• The changes don't fix FSR2 so that it won't choke on render sizes that are below 64px on both dimensions, nor does it address it crashing the editor when the size is below 2px on either of them. I'm not sure if this is due to FSR2's nature or if there is a bug somewhere but if it is the latter, this PR's fix will mask this error and make it harder to trigger, so likely harder to debug and fix in the future. Since the previous behavior can't be toggled back, anyone trying to fix the actual error might be in for a lot of trouble if they don't know about this adjustment.

  • I also never figured out why that specific error was occuring. I feel like it might be related to mipmap issues that occur in relation to FSR2 elsewhere but I didn't manage to find anything to prove or disprove this, so I gave up on investigating the mipmap detail.

• Although the target size is adjusted, if the subviewport is set to 2x2, the preview seems to show just 4 pixels being rendered, when the render size internally should be 64x64 due to the adjustment and so should the target size. I'm not sure what to make of this or if it's a concern but it feels like I may have misunderstood how this whole thing works and I've made the wrong changes. I went through this code with gdb several times but I think the rendering is done on a separate thread so I could only step through the render buffer creation but not what follows it up to the point of rendering (and I'm guessing it would take forever just to step through all that, given how often even _configure_3d_render_buffers() gets triggered even on the UI).

• The new code contains multiple if statements and I'm unsure if I should write it differently to make branch prediction easier. It isn't a particular performance issue in most cases but it might be if someone is just using the slider in the editor, since every change requires going through this code. However, this new code only runs if the dimensions are too small, so for most cases it doesn't run anyway but it might cause failed branch predictions on the cpu. I'm uncertain about whether something needs to be done about this or not. I wrote things in a way that seemed nice to me but i'm not sure if that's the most clear and maintainable way to do it. I would also have liked to do this whole thing branchless, with just one fancy calculation but I couldn't figure out how to do it and even then, it might be a bad idea for maintainability. Plus, the clamps and max macros already run conditionals as it is. Fingers crossed for the compiler to guess correctly and optimise it.

• I would have preferred to make a utility function to do this size adjustment, since it's done once for the render size and once again for the target size and it's the same calculation. It can be done without side-effects, so it would be a pure function and it would make the code more concise and thus easier to read, whereas now it is a block of ifs, but i have no idea where i would put this function or even if it should start with an underscore and things like that. It feels icky because I removed repetitive code only to introduce repetitive code of my own but I was unsure about what is preferred within the codebase.

possible questions:

• "Why not just turn scaling to bilinear when going below 64x64 and be done with it?"
  • This is what I did at some point but FSR2 still has a much sharper look even at these low render sizes, which may be desirable. So I decided it wasn't good enough because there is some Legitimate Use Case (tm) for small viewports using FSR2 instead of bilinear
• "Why not just clamp the render size?"
  • Because that can easily mess up the aspect ratio, which was obvious when I did do this (it was what I tried first) and then went into the editor and played around with the dimensions. The crash and errors were impossible to trigger but once you fell below 64px on either dimension, you got a square render size which, when applied to a non-square viewport size, resulted in squashing and distortion which is very unlikely to feel expected by the user. So it's wasteful but also, wrong.

addendum:

• Figuring out how this worked and having to decide how to fix it made me think that perhaps other adjustments need to be made but those are not decisions I can make alone, so I decide to open up proposals for them but I'm unsure if I should make them in one proposal or separately:
  • Add project setting that limits the maximum texture size dimensions allowed godot-proposals#13085
  • Allow SubViewports to ignore project settings by default godot-proposals#13084

Edit - Added link to spreadsheet that shows how the resizing code behaves from small viewports, high aspect ratios, up to large viewports.

Edit 2 - added link to alternative PR at the top