Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PanicAlert when recording FIFOs #9609

Merged
merged 2 commits into from Mar 27, 2021

Conversation

Pokechu22
Copy link
Contributor

The typo (from f749fcf) was ARRAY_POSITION being used instead of ARRAY_TEXCOORD0.

@@ -42,6 +42,7 @@ enum
CP_COMMAND_MASK = 0xf0,
CP_NUM_VAT_REG = 0x08,
CP_VAT_MASK = 0x07,
CP_NUM_REGULAR_ARRAYS = 12, // Excludes the arrays used for indexed XF loads
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it would make more sense to have this be a separate (constexpr) constant rather than part of the enum, especially since all the other values are written in base 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be a separate constant, or maybe it could be in the enum below. Also, looking at it, perhaps NUM_VERTEX_ARRAYS would be better (as each of those 12 arrays are vertex components). #9540 also adds constants for the indexed XF ones as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd opt for a constant, but a separate enum with only NUM_* constants sounds fine to me too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about the NUM_COLOR_ARRAYS and NUM_TEXCOORD_ARRAYS constants I added next to ARRAY_COLOR0 and ARRAY_TEXCOORD0 in the first commit then?

Copy link
Member

@leoetlino leoetlino Mar 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ambivalent about it -- on the one hand I don't really like indices and sizes/numbers being in the same enum because they're semantically different kinds of values, on the other hand it makes sense to have the number of arrays right next to the starting index 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a separate enum, but it turns out MSVC doesn't like it when you add elements of different enums.

Converting all of those into scoped enums or proper constants (and having them not be in the global namespace) is something that should be done eventually, but will be a bit more refactoring than I want to handle here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. Let's fix the regression and do further cleanup in a followup PR :)

@leoetlino leoetlino merged commit 8fab253 into dolphin-emu:master Mar 27, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants