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

Skylanders: expand and improve character list #12090

Merged
merged 1 commit into from Aug 13, 2023

Conversation

mandar1jn
Copy link
Contributor

@mandar1jn mandar1jn commented Aug 9, 2023

The previous list had some issues. A lot of variant ids were set to 0x0000. Although this works for some figures, on a technicality implemented into the games, they are technically wrong and don't result in exactly the same experience as the real figures. For example, the previous small fry got a "series 1" text on the summon screen. The real small fry does not have this. I also added figure types so I can add separate generation logic later.

The Kaos element only applies to 3 items. So, I decided to throw it under others since it's not listed as an element in the manual and you can easily search for Kaos

The whole list was regenerated from my personal database of figures. Should contain all figures previously listed and should add some more. For example, legendary sky trophy

@mandar1jn mandar1jn marked this pull request as ready for review August 9, 2023 19:49
@mandar1jn mandar1jn force-pushed the figures-split branch 2 times, most recently from ccc696a to cec30d1 Compare August 10, 2023 09:14
Copy link
Contributor

@deReeperJosh deReeperJosh left a comment

Choose a reason for hiding this comment

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

Just added some thoughts, mostly nitpicks but some actual code review too

Source/Core/Core/IOS/USB/Emulated/Skylander.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/USB/Emulated/Skylander.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/USB/Emulated/Skylander.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/USB/Emulated/Skylander.cpp Outdated Show resolved Hide resolved
@mandar1jn
Copy link
Contributor Author

Resolved every comment of @deReeperJosh. Some decisions were made on discord in DM's. Happy to elaborate if needed

Copy link
Contributor

@deReeperJosh deReeperJosh left a comment

Choose a reason for hiding this comment

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

Some more review comments from me, might also want to change the width of all the radio buttons/checkboxes to be the same width as each other. As well, maybe change the default size of the window as it looks slightly squashed with the new type radio that has been added:
Screenshot 2023-08-12 at 9 24 49 PM

@deReeperJosh
Copy link
Contributor

@jnaidu360 seeing as you first worked on this updated window, feel free to add any reviews/thoughts here too :)

@mandar1jn
Copy link
Contributor Author

Some more review comments from me, might also want to change the width of all the radio buttons/checkboxes to be the same width as each other. As well, maybe change the default size of the window as it looks slightly squashed with the new type radio that has been added: Screenshot 2023-08-12 at 9 24 49 PM

I can replicate the width. Need to figure out why that is different. Literally copy pasted that from the element radio. I can't, however, replicate that extremely cramped look of the options. Windows seems to display those differently.
image

@deReeperJosh
Copy link
Contributor

Might just be a MacOS Qt thing, feel free to ignore the height comment

@mandar1jn
Copy link
Contributor Author

Okay. I have too little QT experience to fix the width of the filters. I can't get the options to align in the middle

The previous list had some issues. A lot of variant id's were set to 0x0000. Althought this works for some figures, on a technicallity implemented into the games, they are technically wrong and don't result in exactly the same experience as the real figures. For example, the previous small fry got a "series 1" text in the summon screen. The real small fry does not have this. I also added figure types so I can add seperate generation logic later.
The Kaos element only applies to 3 items. So, I decided to throw it under others since it's not listed as an element in the manual and you can easily search for Kaos
@mandar1jn
Copy link
Contributor Author

@deReeperJosh every filter should have the same width now. I aligned the radio's to the center. I also fixed your scrollbar issue. There should only be a scrollbar for the figure list on minimum width now

@deReeperJosh
Copy link
Contributor

Yup, looks much better now! LGTM

@JMC47 JMC47 merged commit 4549091 into dolphin-emu:master Aug 13, 2023
11 checks passed
@mandar1jn mandar1jn deleted the figures-split branch August 22, 2023 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants