Graphics refactoring + add names and descriptions in FIFO analyzer #9497

Pokechu22 · 2021-02-09T01:33:22Z

The big thing here is names and detailed descriptions for most XF, BP, and CP registers, and the names being shown in the overview, along with refactoring of the graphics code to use BitField and enum class. ~~Additionally, the way the frame and object ranges work has been tweaked to be inclusive (for frames) and not require selecting 2 objects.~~

For the most part, I've done the descriptions by creating custom fmt::formatters, and then using fmt::format or fmt::to_string on the objects. I construct the objects to print using e.g. LitChannel{.hex = value} (since most of them do not have constructors). For enums, I've also introduced a new EnumFormatter that makes it easy to create fmt::formatters for the enums (including clean handling of invalid values). I also tried to expand most of the abbreviations, but I may have done some of them wrong. There may be typos or other mistakes in the results.

Screenshots

Two things that I'd like to do that I haven't included here: 1, Show state changes before the draw instead of after (see the unintuitive warning in the wiki), and 2, make the FIFO player a separate window that can be shown over the render window (currently, at least on Windows, the main dolphin window will always show up behind the FIFO player, blocking the render window). Further changes to the FIFO analyzer itself are found in #9540, which has a weak dependency on this PR.

Source/Core/Core/FifoPlayer/FifoPlayer.cpp

Source/Core/DolphinQt/FIFO/FIFOAnalyzer.cpp

Source/Core/VideoCommon/BPStructs.cpp

Source/Core/VideoCommon/XFStructs.h

Source/Core/VideoCommon/XFStructs.cpp

Source/Core/DolphinQt/FIFO/FIFOAnalyzer.cpp

iwubcode · 2021-02-09T04:35:47Z

Source/Core/VideoCommon/XFStructs.cpp

+      break;
+    }
+    return format_to(ctx.out(),
+                     "Projection: {}\nInput form: {}\nTex gen type: {}\n"


Could we call this 'Size' instead of 'Projection'? I think of different things when I hear 'projection' but maybe I'm misunderstanding

YAGCD calls it projection, while libogc's GX_SetTexCoordGen calls it tgen_typ and has GX_TG_MTX3x4 and GX_TG_MTX2x4; the 3 by 4 version divides by Q which I think is the actual projection step. But I don't think calling it projection is particularly obvious (nor is typ(e)); size might be better.

iwubcode · 2021-02-09T04:39:10Z

Source/Core/VideoCommon/XFStructs.cpp

+                          fmt::format("Number of color channels: {}", value & 3));
+    break;
+
+  case XFMEM_SETCHAN0_AMBCOLOR:  // Channel Ambient Color


Too bad we can't somehow pass the color from here to Qt. It'd be nice to somehow see the color in addition to the hex code

I guess we could use rich text tags like I did for the tooltips. Kinda muddies the description text though..

Source/Core/VideoCommon/XFStructs.cpp

Source/Core/VideoCommon/BPStructs.cpp

Pokechu22

This is much closer to a final version (though I still need to rebase and tidy up the commits).

I've converted a lot of graphics code to use BitField and enum classes, and have created fmt::formatters for most of those enums (and a EnumFormatter to simplify creating such fomatters) and the corresponding structures. Those formatters are used in the fifo analyzer but also can be used in logging, which is fairly convenient (and thus I've put them into the header file instead of the cpp files used by the fifo analyzer).

One issue caused by the use of enum class is that there are many std::arrays that map some enum into different host API equivalents or code snippets for shaders. These can't be indexed using an enum class, so I need to cast them; I've been doing e.g. u32(state.logicmode.Value()) (the Value() part is needed because of BitField; it cannot cast directly to u32). This is a bit messy, but I'm not sure if there's a better solution.

Pokechu22 · 2021-02-12T07:01:52Z

Source/Core/VideoCommon/RenderBase.cpp

@@ -1697,7 +1697,7 @@ bool Renderer::UseVertexDepthRange() const
    return false;

  // We need a full depth range if a ztexture is used.
-  if (bpmem.ztex2.type != ZTEXTURE_DISABLE && !bpmem.zcontrol.early_ztest)
+  if (bpmem.ztex2.op != ZTexOp::Disabled && !bpmem.zcontrol.early_ztest)


This seems to have been a typo originally; type was the format (previously TEV_ZTEX_TYPE_U8/_U16/_U24). I'm not sure if that affected anything though.

Source/Core/VideoCommon/BPMemory.h

Pokechu22 · 2021-02-12T07:08:02Z

Source/Core/VideoCommon/BPMemory.h

+  BitField<12, 3, u32> bi2;
+  BitField<15, 3, u32> bc3;  // Typo?
+  BitField<18, 3, u32> bi4;
+  BitField<21, 3, u32> bc4;


These names are somewhat odd since it skips bc2 and bi3; I don't think it should have gone up to 4 at all. I've left this unchanged in case there's some intended meaning though.

Pokechu22 · 2021-02-12T07:11:41Z

Source/Core/VideoCommon/RenderState.cpp

+  // TODO: This previously had a warning about SRC and DST being aliased and not to mix them,
+  // but INVSRCCLR and INVDSTCLR were also aliased and were mixed.
+  // Thus, NOR, EQUIV, INVERT, COPY_INVERTED, and OR_INVERTED duplicate(d) other values.


I split BlendFactor into SrcBlendFactor and DstBlendFactor since DstClr/InvDstClr and SrcClr/InvSrcClr have differences in behavior and are worth treating differently, and I found that some entries in this table use InvDstClr/InvSrcClr even though they're not actually available. I'm not sure exactly what to do here, but the behavior here may be (and have been) wrong.

Pokechu22 · 2021-02-12T07:12:54Z

Source/Core/DolphinNoGUI/PlatformX11.cpp

-  Cursor m_blank_cursor = None;
+  Cursor m_blank_cursor = 0L;  // None


This is some jank due to me needing to undefine None which X.h defines to be 0L, since that define interferes with some enums. I'm not sure if there's a better workaround.

PatrickFerry · 2021-02-12T18:02:53Z

Note:
Fifoplayer is crashing to desktop with this PR. (I am using Windows 10)

As an example: https://bugs.dolphin-emu.org/attachments/8076/nes%20nekkotsu.dff

Pokechu22 · 2021-02-12T18:28:06Z

More specifically it plays correctly the first time, but after stopping playback, trying to play again or analyze the opened FIFO causes a crash. (I'm guessing that that's also what's breaking the radeon fifoci builder, since it's segfaulting, but I haven't confirmed).

EDIT: The crash when analyzing the opened FIFO after stopping playback also happens on 5.0-13603, but I seem to have introduced the crash on playing again in this PR in 650b236.

Pokechu22 · 2021-02-12T21:47:55Z

Source/Core/VideoCommon/VertexLoader.cpp

-      ASSERT_MSG(VIDEO, FORMAT_UBYTE <= format && format <= FORMAT_FLOAT,
-                 "Invalid texture coordinates format!\n(format = %d)", format);
-      ASSERT_MSG(VIDEO, 0 <= elements && elements <= 1,
-                 "Invalid number of texture coordinates elements!\n(elements = %d)", elements);
+      ASSERT_MSG(VIDEO, ComponentFormat::UByte <= format && format <= ComponentFormat::Float,
+                 "Invalid texture coordinates format!\n(format = %d)", (u32)format);
+      ASSERT_MSG(VIDEO, elements == TexComponentCount::S || elements == TexComponentCount::ST,
+                 "Invalid number of texture coordinates elements!\n(elements = %d)", (u32)elements);


Note the (u32)format here; ideally I'd just let the enum formatter handle this, but there doesn't seem to be an fmt equivalent of ASSERT_MSG.

Pokechu22 · 2021-02-13T03:10:27Z

I've fixed a typo that was causing rendering differences (044b078), including slightly different rendering in Need For Speed: Underground (JMC confirmed that the blue windows are what show up on hardware, even though they do look somewhat worse). I've also rebased to the latest master, which includes #9501, so FifoCI should run properly now.

I've also fixed the crashes with the fifo player. One (which triggered on playing back a fifolog a second time) was introduced in 17186b0, and fixed in a127320 by fixing a few places where I didn't update to m_File->GetFrameCount() - 1 for inclusive maximums. A second occurred when trying to analyze the fifolog after playback stopped and the file was closed; this was fixed in 9a32a13 (note that the call to m_analyzer->Update() sets the text to "No recording loaded.", but the file is closed a bit before that happens, so the checks for FifoPlayer::GetInstance().IsPlaying() are also needed). That bug affected 5.0-13603. I also (hopefully) fixed a 3rd bug (in a23b638), where on stopping playback the player would deadlock in WriteFifo, waiting for IsHighWatermarkSet; this was much less consistent but I did manage to reproduce it in 5.0-13603.

Pokechu22 · 2021-02-18T08:18:26Z

I've created a new BitFieldArray type that can be used to access multiple sequential bitfields (for instance, the Tex0Coord-Tex7Coord fields in TVtxDesc), and I've also exposed BitField's StorageType (it still defaults to the same thing it did before, but now it can be manually specified, allowing BitFields of bool).

I've also changed the fifo player window to be a separate window (that can be maximized and doesn't cause the main window to cover the render window), and done a bit more BitField-related adjustment of some of the video registers (in particular, TVtxDesc now uses BitField, and is packed in a way that matches how the values are stored on console (17 and 16 bits) instead of being packed tightly (32 bits and 1 bit); this was changed in a way that should hopefully not break savestates or shader UIDs though I haven't tested it).

What remains before I mark this ready for review is fixing the most recent compile errors and fifoci regressions, and then rebasing everything so that it's in a more sane series of commits where each builds and the changes are specific and complete (so that the massive diff isn't as much of an issue).

Source/Core/VideoCommon/UberShaderVertex.cpp

Source/Core/VideoCommon/VertexLoaderX64.cpp

iwubcode · 2021-03-06T23:32:42Z

Source/Core/VideoBackends/Software/Clipper.cpp

@@ -489,13 +489,15 @@ bool CullTest(const OutputVertexData* v0, const OutputVertexData* v1, const Outp

  backface = normalZDir <= 0.0f;

-  if ((bpmem.genMode.cullmode & 1) && !backface)  // cull frontfacing
+  if ((bpmem.genMode.cullmode == CullMode::Back || bpmem.genMode.cullmode == CullMode::All) &&


These enum choices are odd with the comment. Just checking to make sure that's intentional?

The mask is correct (1 is back, 2 is front, 3 is all), but the behavior seems odd to me. YAGCD says that the bit is for what should be rejected (REJECT_EN in GEN_MODE near §5.11.1). It's possible that the software renderer's definition of backface is reversed from the hardware renderer's, which was canceled out here. (The changes needed for #9350 were less than I expected at least).

I've added a TODO comment for this; it might be an inconsistency with the software renderer but probably shouldn't be addressed here.

iwubcode · 2021-03-06T23:37:17Z

Source/Core/VideoCommon/UberShaderPixel.cpp

@@ -367,21 +367,21 @@ ShaderCode GenPixelShader(APIType ApiType, const ShaderHostConfig& host_config,
      "// which are common to both color and alpha channels\n"
      "bool tevCompare(uint op, int3 color_A, int3 color_B) {{\n"
      "  switch (op) {{\n"
-      "  case 0u: // TEVCMP_R8_GT\n"
+      "  case 0u: // TevCompareMode::R8, TevComparison::GT\n"


Surprised these aren't using the case {:s}:\n trick

It's because there are two enums, instead of one pseudo-enum/set of random values that don't really make sense. The values were previously these (note how they don't match what's in the switch statement):

TEVCMP_R8_GT = 8, TEVCMP_R8_EQ = 9, TEVCMP_GR16_GT = 10, TEVCMP_GR16_EQ = 11, TEVCMP_BGR24_GT = 12, TEVCMP_BGR24_EQ = 13, TEVCMP_RGB8_GT = 14, TEVCMP_RGB8_EQ = 15, TEVCMP_A8_GT = TEVCMP_RGB8_GT, TEVCMP_A8_EQ = TEVCMP_RGB8_EQ

and the enum was used by ((cc.shift << 1) | cc.op | 8). I decided to just split it up into two enums; for the software renderer I was able to split it into two parts (acting on cc.compare and then looking at whether it was using eq or gt), but for the hardware backends I left it in the combined form since it would require further restructuring for no real gain.

It does look like there are other switch statements in this file that hardcode enum values, though; those could be changed.

Due to the binary search tree structure used for the other enums (in addition to the switch statements), I've decided not to deal with the hardcoded enum values in this PR; it can be handled in a later one.

Source/Core/DolphinLib.vcxproj

Additionally, VCacheEnhance has been added to UVAT_group1. According to YAGCD, this field is always 1. TVtxDesc also now has separate low and high fields whose hex values correspond with the proper registers, instead of having one 33-bit value. This change was made in a way that should be backwards-compatible.

Additionally a new ClipDisable union has been added (though it is not currently used by Dolphin).

This value will be used in the register description; so expose it in a way that can be re-used instead of calculating it in 2 places later.

Additional changes: - For TevStageCombiner's ColorCombiner and AlphaCombiner, op/comparison and scale/compare_mode have been split as there are different meanings and enums if bias is set to compare. (Shift has also been renamed to scale) - In TexMode0, min_filter has been split into min_mip and min_filter. - In TexImage1, image_type is now cache_manually_managed. - The unused bit in GenMode is now exposed. - LPSize's lineaspect is now named adjust_for_aspect_ratio.

BPMEM_TEV_COLOR_ENV + 6 (0xC6) was missing due to a typo. BPMEM_BP_MASK (0xFE) does not lend itself well to documentation with the current FIFO analyzer implementation (since it requires remembering the values in BP memory) but still shouldn't be treated as unknown. BPMEM_TX_SETMODE0_4 and BPMEM_TX_SETMODE1_4 (0xA4-0xAB) were missing entirely.

lioncash reviewed Feb 9, 2021

View reviewed changes

Source/Core/DolphinQt/FIFO/FIFOAnalyzer.cpp Outdated Show resolved Hide resolved

Pokechu22 force-pushed the better-fifo-analyzer branch 2 times, most recently from 24b7bc5 to 66bd5da Compare February 9, 2021 04:20

iwubcode reviewed Feb 9, 2021

View reviewed changes

Source/Core/VideoCommon/XFStructs.cpp Outdated Show resolved Hide resolved

iwubcode reviewed Feb 9, 2021

View reviewed changes

Source/Core/VideoCommon/BPStructs.cpp Outdated Show resolved Hide resolved

Pokechu22 force-pushed the better-fifo-analyzer branch 5 times, most recently from b85d6ef to 673a949 Compare February 11, 2021 03:09

Pokechu22 marked this pull request as draft February 12, 2021 00:55

Pokechu22 force-pushed the better-fifo-analyzer branch from bee8cb3 to f9d5564 Compare February 12, 2021 05:48

Pokechu22 commented Feb 12, 2021

View reviewed changes

Pokechu22 force-pushed the better-fifo-analyzer branch from 548f70c to d67ca33 Compare February 12, 2021 18:57

Pokechu22 commented Feb 12, 2021

View reviewed changes

Pokechu22 force-pushed the better-fifo-analyzer branch from 9b01626 to a23b638 Compare February 13, 2021 02:55

Pokechu22 force-pushed the better-fifo-analyzer branch 3 times, most recently from 267ee7f to 7b3a289 Compare February 16, 2021 22:03

Pokechu22 force-pushed the better-fifo-analyzer branch 4 times, most recently from c915a69 to e4bda20 Compare February 20, 2021 21:23

iwubcode reviewed Mar 6, 2021

View reviewed changes

Source/Core/VideoCommon/UberShaderVertex.cpp Outdated Show resolved Hide resolved

iwubcode reviewed Mar 6, 2021

View reviewed changes

Source/Core/VideoCommon/UberShaderVertex.cpp Outdated Show resolved Hide resolved

Pokechu22 commented Mar 6, 2021

View reviewed changes

Source/Core/VideoCommon/VertexLoaderX64.cpp Outdated Show resolved Hide resolved

iwubcode reviewed Mar 6, 2021

View reviewed changes

Pokechu22 added 3 commits March 6, 2021 17:34

Create BitFieldArray

f697e17

DolphinNoGUI/PlatformX11: Work around X.h's None being undefined

d702f3b

Create constants for CP registers and masks

c27efb3

Pokechu22 force-pushed the better-fifo-analyzer branch 3 times, most recently from 2d3771d to 53bd7ac Compare March 7, 2021 03:04

iwubcode reviewed Mar 7, 2021

View reviewed changes

Source/Core/DolphinLib.vcxproj Outdated Show resolved Hide resolved

Pokechu22 added 11 commits March 6, 2021 19:27

Add names and descriptions for CP registers to the FIFO analyzer

953e094

Convert XFMemory to BitField and enum class

aab81d5

Additionally a new ClipDisable union has been added (though it is not currently used by Dolphin).

Add names and descriptions for XF registers to the FIFO analyzer

2d6ec74

Add names and descriptions for regular XF memory

8c80369

Use XFMEM_REGISTERS_START/END in XFRegWritten and LoadXFReg

81b84a5

Rename BPMEM_EFB_BR to BPMEM_EFB_WH

762fe33

Fix typo with ztex2 op in UseVertexDepthRange

f2bea67

Add FogParam0::FloatValue and FogParam3::FloatValue

db8ced7

This value will be used in the register description; so expose it in a way that can be re-used instead of calculating it in 2 places later.

Pokechu22 force-pushed the better-fifo-analyzer branch from 53bd7ac to df81210 Compare March 7, 2021 03:28

iwubcode approved these changes Mar 7, 2021

View reviewed changes

JMC47 merged commit 089250f into dolphin-emu:master Mar 7, 2021

This was referenced Mar 7, 2021

EnumFormatter: fix signed/unsigned comparison warnings #9574

Merged

Add game quirks for unknown BP/CP/XF commands #9576

Merged

This was referenced Apr 20, 2021

VideoCommon: Fix scissorOffset, handle negative value correctly #9656

Closed

Fix out of bounds tex coord behavior; always apply fb_addprev and tex coord wrapping #9651

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Graphics refactoring + add names and descriptions in FIFO analyzer #9497

Graphics refactoring + add names and descriptions in FIFO analyzer #9497

Pokechu22 commented Feb 9, 2021 •

edited

iwubcode Feb 9, 2021 •

edited

Pokechu22 Feb 9, 2021 •

edited

iwubcode Feb 9, 2021

iwubcode Feb 28, 2021

Pokechu22 left a comment

Pokechu22 Feb 12, 2021

Pokechu22 Feb 12, 2021

Pokechu22 Feb 12, 2021

Pokechu22 Feb 12, 2021

PatrickFerry commented Feb 12, 2021 •

edited

Pokechu22 commented Feb 12, 2021 •

edited

Pokechu22 Feb 12, 2021

Pokechu22 commented Feb 13, 2021

Pokechu22 commented Feb 18, 2021

iwubcode Mar 6, 2021

Pokechu22 Mar 7, 2021

Pokechu22 Mar 7, 2021

iwubcode Mar 6, 2021

Pokechu22 Mar 7, 2021

Pokechu22 Mar 7, 2021

		Cursor m_blank_cursor = None;
		Cursor m_blank_cursor = 0L; // None

Graphics refactoring + add names and descriptions in FIFO analyzer #9497

Graphics refactoring + add names and descriptions in FIFO analyzer #9497

Conversation

Pokechu22 commented Feb 9, 2021 • edited

iwubcode Feb 9, 2021 • edited

Choose a reason for hiding this comment

Pokechu22 Feb 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pokechu22 left a 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

PatrickFerry commented Feb 12, 2021 • edited

Pokechu22 commented Feb 12, 2021 • edited

Choose a reason for hiding this comment

Pokechu22 commented Feb 13, 2021

Pokechu22 commented Feb 18, 2021

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

Pokechu22 commented Feb 9, 2021 •

edited

iwubcode Feb 9, 2021 •

edited

Pokechu22 Feb 9, 2021 •

edited

PatrickFerry commented Feb 12, 2021 •

edited

Pokechu22 commented Feb 12, 2021 •

edited