nv2a: Update `inline_value`s as rendering side-effect #623

abaire · 2022-01-08T02:22:09Z

This updates the default inline_value for the normal vertex attribute whenever valid values for the normal are passed, mirroring the hardware behavior observed in https://github.com/abaire/nxdk_pgraph_tests/blob/main/tests/lighting_normal_tests.cpp.

Additional tests for all attributes:

test
results

I'm not entirely confident that this resolves all possible paths, but it does fix #601

(note: screenshot was from my dev branch that also includes fix for #599, without which the minimap would be opaque but the lower left panel would still be fixed)

Shoegzer · 2022-01-11T03:45:46Z

With this PR, JSRF crashes immediately prior to the main menu (after the loading screen), with the following message in term:

qemu-system-i386: ../hw/xbox/nv2a/pgraph.c:6273: pgraph_bind_vertex_attributes: Assertion attr->gl_type == GL_FLOAT && "Need to convert non-float normal."' failed.

abaire · 2022-01-11T04:15:00Z

I'm hoping that I'm being overly cautious with that assert so it can probably be removed (though I'm interested in seeing what is actually being used if it's not a float).

Unfortunately it seems like I can no longer get past the loading screen even with a clean build from master so I'm putting this back into draft mode until I can verify that it's safe to remove the assert.

abaire · 2022-01-11T05:16:37Z

Looks like it uses unsigned byte for normals after the loading screen. Removed the assert and fixed the handling to support arbitrary types instead of assuming they'll always be floats..I also had an error in how I was grabbing the last element, so as an added bonus the coloring is more correct now as well.

Thanks for testing @Shoegzer !

abaire · 2022-01-11T05:28:43Z

hw/xbox/nv2a/pgraph.c

+                uint32_t element_size = attr->size * attr->count;
+                const uint8_t* last_entry = d->vram_ptr + start;
+                if (stride) {
+                    last_entry += stride * (num_elements - 2);


Double checking on these. Is it possible that num_elements doesn't actually need the +1 at the top of the function (I'd expect that these should be num_elements - 1, but that definitely reads past the last element)?

Not sure if there's an off-by-one error or just an error in my assumptions based on the naming.

In my pgraph test I'm sending a single triangle (3 elements).

count is decremented by one before being sent to DRAW_ARRAYS as per spec, but that is handled here so pg->draw_arrays_max_count ends up being correct (3)

When the BEGIN_END is ended, the vertex attributes are bound w/ 3 as max_element. The naming and the +1 makes it sound like max_element is expected to be the maximum valid element (2 in my test case) rather than the max count (3) that's being passed in.

Testing with Gun Valkyrie I feel more confident that the - 2 is wrong and the DRAW_ARRAYS path should be passing max_element rather than count. Added a fix for that in this PR.

Shoegzer · 2022-01-12T02:46:43Z

Tested and confirmed the assert/crashing is gone, and the bottom-left panel looks good. I haven't merged your other fix for the map itself, but with this PR alone it now shows:

Nice work!

abaire · 2022-01-22T20:51:24Z

Just happened to find that this crashes Gun Valkyrie, where a call is made with num_elements == 1

Shoegzer · 2022-01-22T21:16:01Z

That's too bad. Not sure what that means in terms of getting this PR merged, though from what you say here:

I also had an error in how I was grabbing the last element, so as an added bonus the coloring is more correct now as well.

Would the panel color still be mostly correct if you avoided the condition for num_elements == 1? If so, would it make any sense to move that part to another PR and merge this one? Happy to help test any workarounds either way.

abaire · 2022-01-22T21:30:12Z

hw/xbox/nv2a/pgraph.c

@@ -2502,7 +2502,7 @@ DEF_METHOD(NV097, SET_BEGIN_END)
            assert(pg->inline_elements_length == 0);

            pgraph_bind_vertex_attributes(d, pg->draw_arrays_min_start,
-                                          pg->draw_arrays_max_count, false, 0);
+                                          pg->draw_arrays_max_count - 1, false, 0);


Based on the usage in pgraph_bind_vertex_attributes, it looks like the expectation is that this should be the last used element rather than the count.

Setting this to count requires a -2 to access the last used element in JSRF (and causes crashes in cases that use inline elements, such as Gun Valkyrie). As far as I can see, providing max_element in this path still works correctly in the games I've tested.

abaire · 2022-01-22T22:01:59Z

That's too bad. Not sure what that means in terms of getting this PR merged, though from what you say here:

I also had an error in how I was grabbing the last element, so as an added bonus the coloring is more correct now as well.

Would the panel color still be mostly correct if you avoided the condition for num_elements == 1? If so, would it make any sense to move that part to another PR and merge this one? Happy to help test any workarounds either way.

I think the crash was due to me working around what I think is a bug elsewhere. To fix the crash I fixed the other bug, so I think this should still be merge-able. It'd be great if you could verify that it still works for you with this latest push and try out as many other games as you can to see if it causes any weird glitches. The underlying fix will affect many more things than lighting but ideally you would not see any actual difference.

Shoegzer · 2022-01-22T23:11:28Z

You bet. Tested JSRF and it still works fine. I also tested Halo:CE, Halo 2 and those work as well as before at least. Star Wars KOTOR regressed, however, as eyes are missing from the character select screen (though this issue occurred before until very recently):

PR:

Master:

Side-note since you asked: I have many other discs but I don't want to dump them to non-archival quality (xiso) images for various reasons as mentioned in #238 or I'd test those for you too. Hopefully xemu can support proper isos in the future (though I'm patient and respect priorities).

abaire · 2022-01-22T23:53:38Z

Ah, that'd almost certainly mean that I'm wrong about the count versus max somehow, or that there's some interesting dependency on the draw mode that needs to be accounted for. I own KOTOR so I'll do some debugging in a few days when I'm back with my hardware.

abaire · 2022-01-24T17:46:47Z

Well, the good news is that this seems to have been a simple case of the fix branch being older than the fix for KOTOR. I rebased to the latest master and the eyes now look correct. @Shoegzer would you mind re-verifying with the latest push?

The bad news is that I wrote a much more in-depth test in anticipation of tracking this down and found that the behavior in the case of inline elements is slightly wrong (it should always take the last rendered vertex, the code I wrote takes the last buffered vertex). I also verified that all the other attributes was able to test are supposed to carry over in the same way the normals do (diffuse, specular, tex coords, almost certainly the rest but I can't easily test them with nxdk's Cg-based shaders).

@mborgerson would you be OK to review and merge this PR as is and I'll do a followup?
The handling of the inline elements and additional attributes will add some complexity so I'm inclined to get this part that actually fixes a game in and then expand on it (I don't know if any other games suffer similar problems due to missing carryover).

mborgerson · 2022-01-24T18:53:57Z

The bad news is that I wrote a much more in-depth test in anticipation of tracking this down and found that the behavior in the case of inline elements is slightly wrong (it should always take the last rendered vertex, the code I wrote takes the last buffered vertex). I also verified that all the other attributes was able to test are supposed to carry over in the same way the normals do (diffuse, specular, tex coords, almost certainly the rest but I can't easily test them with nxdk's Cg-based shaders).

This is what I had expected. Thanks for creating a test case, it is actually good news.

@mborgerson would you be OK to review and merge this PR as is and I'll do a followup? The handling of the inline elements and additional attributes will add some complexity so I'm inclined to get this part that actually fixes a game in and then expand on it (I don't know if any other games suffer similar problems due to missing carryover).

Let's just fix it generally, it should not be too difficult.

By the way, the arrays implementation is flawed. I have a patch for it that notably fixes some sports games, but it needs a couple of remaining improvements. You are welcome to work on it, or I'll return to it shortly

abaire · 2022-01-24T20:36:01Z

The bad news is that I wrote a much more in-depth test in anticipation of tracking this down and found that the behavior in the case of inline elements is slightly wrong (it should always take the last rendered vertex, the code I wrote takes the last buffered vertex). I also verified that all the other attributes was able to test are supposed to carry over in the same way the normals do (diffuse, specular, tex coords, almost certainly the rest but I can't easily test them with nxdk's Cg-based shaders).

This is what I had expected. Thanks for creating a test case, it is actually good news.

@mborgerson would you be OK to review and merge this PR as is and I'll do a followup? The handling of the inline elements and additional attributes will add some complexity so I'm inclined to get this part that actually fixes a game in and then expand on it (I don't know if any other games suffer similar problems due to missing carryover).

Let's just fix it generally, it should not be too difficult.

By the way, the arrays implementation is flawed. I have a patch for it that notably fixes some sports games, but it needs a couple of remaining improvements. You are welcome to work on it, or I'll return to it shortly

abaire · 2022-01-24T20:43:29Z

Accidentally closed, sorry! This should be ready for review now.

Let's just fix it generally, it should not be too difficult.

Sounds good. PR is updated, all testable cases now work correctly. I can't fully test the inline buffers case because xemu doesn't yet handle all of the attribute setter functions (e.g., SET_DIFFUSE_COLOR w/ 4 floats which my test program uses because I haven't gotten around to making use of floats versus DWORDs conditional).

By the way, the arrays implementation is flawed. I have a patch for it that notably fixes some sports games, but it needs a couple of remaining improvements. You are welcome to work on it, or I'll return to it shortly

Cool, I can take a look, I assume it's fix/arrays? Do you know offhand which games it fixes?

While writing this PR I noticed that there's an off-by one in the call to pgraph_bind_vertex_attributes when handling DRAW_ARRAYS but I've stomped on my comments a couple times now. Mentioning it here in case you haven't read the full conversation history.

Triticum0 · 2022-01-24T22:01:08Z

It is fix/array and the game it fixes are on this issue #329.

Shoegzer · 2022-01-24T22:37:30Z

Well, the good news is that this seems to have been a simple case of the fix branch being older than the fix for KOTOR. I rebased to the latest master and the eyes now look correct. @Shoegzer would you mind re-verifying with the latest push?

I was wondering if this was a rebase issue since the eyes were fixed so recently in master, glad to know it was that simple. In any event, KOTOR eyes are fine now with your PR, and of course JSRF lower-left panel looks good too. Nice.

hw/xbox/nv2a/pgraph.c

mborgerson · 2022-02-22T21:56:20Z

Thanks!

abaire marked this pull request as draft January 11, 2022 03:58

abaire force-pushed the updates_normal_inline_value branch 2 times, most recently from 356a740 to e01bd12 Compare January 11, 2022 05:13

abaire marked this pull request as ready for review January 11, 2022 05:16

abaire commented Jan 11, 2022

View reviewed changes

abaire marked this pull request as draft January 22, 2022 20:50

abaire force-pushed the updates_normal_inline_value branch 2 times, most recently from 534a2d5 to b90551c Compare January 22, 2022 21:27

abaire commented Jan 22, 2022

View reviewed changes

abaire marked this pull request as ready for review January 22, 2022 21:31

abaire marked this pull request as draft January 22, 2022 23:52

abaire force-pushed the updates_normal_inline_value branch from b90551c to e684395 Compare January 24, 2022 17:40

abaire force-pushed the updates_normal_inline_value branch 2 times, most recently from 5656a56 to 10b29fc Compare January 24, 2022 20:31

abaire closed this Jan 24, 2022

abaire reopened this Jan 24, 2022

abaire marked this pull request as ready for review January 24, 2022 20:36

This was referenced Jan 24, 2022

pgraph: Several inline buffer vertex attribute setters are not implemented #671

Closed

nv2a: Implement vertex attribute setters #672

Merged

abaire force-pushed the updates_normal_inline_value branch from 10b29fc to 409b2ab Compare January 24, 2022 23:39

abaire changed the title ~~nv2a: Update normal inline_value as rendering side-effect~~ nv2a: Update inline_values as rendering side-effect Jan 28, 2022

mborgerson reviewed Feb 11, 2022

View reviewed changes