Skylanders Portal Menu Navigational Improvements

[jnaidu360]

Status

Ready to merge.

Description

The current portal menu seems to break immersion. I'm trying to overhaul the portal menu to help preserve immersion during gameplay. Mainly, the goal is to make navigation easier while preserving existing functionality. I added several features:

• Search menu for Skylanders
• Filters by release title (eg. Spyro's Adventure, Giants) and by element (eg. magic, fire)
• Automatic file creation and loading
• In-game button to open portal menu, to prevent having to switch tabs during gameplay (using hotkeys instead)

Notes

Upon opening, the portal window now requests the creation of directory named "Skylanders" within the user directory, if one does not exist. This directory is used as the default path for Skylanders collection, where .sky files are created, saved, and loaded.

The filters for the search window were sorted manually. The SkylanderFilters class is scalable in case more filters should be added.

[Dentomologist]

Thank you for submitting the pull request, but creating a new one and closing the old one each time you make changes to it is a bit disruptive. Could you describe the tool/process you're using to submit the requests so we can guide you on how to update them?

[jnaidu360]

Sorry about that, I won't do that anymore. I switched repositories because the first one wasn't organized properly. I created the pull request from the GitHub website.

[Rumi-Larry]

I don't know how easy or useful this would be, but it would make sense for both lists to be comboboxes, with a button to select or deselect every option. Then again maybe it doesn't make sense to filter out (for instance) all water, magic and tech from all games

[jnaidu360]

That's not a bad idea, it would certainly add functionality. The reason element selection uses radio buttons right now is for the reason you said- I couldn't think of a situation when multiple elements would need to be selected. My guess is checkboxes would get annoying to uncheck, but I can definitely test it out.

[Florin9doi]

Why wouldn't you store the game/element within list_skylanders?

enum SkyGame { G_SPYROS_ADV, G_GIANTS, G_SWAP_FORCE, G_TRAP_TEAM, G_SUPERCHARGERS };
enum SkyElement { E_MAGIC, E_WATER, E_TECH, E_FIRE, E_EARTH, E_LIFE, E_AIR, E_UNDEAD, E_OTHER };
struct SkyCharacter { const char* name; const SkyGame game; const SkyElement element; };
const std::map<const std::pair<const u16, const u16>, const SkyCharacter> list_skylanders = {
    {{16, 0x0000}, {"Spyro", G_SPYROS_ADV, E_MAGIC}},
    {{104, 0x0000}, {"Hot Head", G_GIANTS, E_FIRE}},
    {{473, 0x0000}, {"Tread Head", G_TRAP_TEAM, E_TECH}},
...
};

[jnaidu360]

That's way better, I should've done that originally. I'll make the change.

[deReeperJosh]

Just left a few things to start, appreciate that if you don't have a Mac to debug some of them there will be no immediate fix, but I love the look of the new UI. Definitely much nicer than buttons and text! I've also attached a SS so you can see what I mean.

[deReeperJosh]

Also, just as some feedback based on my experience with my first commits: Always run clang formatting on files you have changed before committing, when you need to squash commits, I recommend Github Desktop. Made my life 10x easier. And also, make sure to rebase from the upstream branch whenever there are changes. Every so often, running git pull --rebase master. If you need/want help, feel free to reach out!

[jnaidu360]

Awesome, thanks for the help. I'm getting to work on it.

[jnaidu360]

Hey @deReeperJosh, I think I've fixed the issues you found except the ones relating to Mac. I found access to a Mac at a library nearby and I'm trying to build Dolphin so I can test. However I'm running into a problem when running CMake. I don't have admin on this computer so that might be part of the problem, but I'll attach the command line output if it helps. What do you recommend?

Command line output.zip

[deReeperJosh]

@jnaidu360 looks like you just need to run the command that it mentions, git submodule update --init --recursive rather than just git submodule update --init

[jnaidu360]

@deReeperJosh Thanks for the fast response. Now I get a message that "FindQt.cmake" isn't in CMAKE_MODULE_PATH. I'm looking in Dolphin's CMake folder and there's no FindQt.cmake file. Do I have to create it myself?

Command line output 2.zip

[deReeperJosh]

@jnaidu360 yeah you will need to install the package manager with admin rights for this step, I am assuming you haven't installed homebrew?

[jnaidu360]

Correct. Let me figure out a way to get a Mac with admin and I'll get back to you.

[deReeperJosh]

I'm happy to try to make any changes you're after on my machine, and can contribute to your repo if that's easier?

[jnaidu360]

I'm not at all opposed to that but I'd like to try on my own first if that's ok. I see it as a learning experience 😋

[jnaidu360]

@deReeperJosh OK, I finally have access to a Mac. What is it specifically I need to do? I have the same exact problem as before. CMake is installed properly, I updated submodules, but it doesn't see Qt?

Edit: I have Homebrew as well.

[deReeperJosh]

Cool, as long as you install homebrew and then the necessary packages using homebrew you should be fine, just need to use the same commands specified in the build instructions on the readme.

Run brew install qt git cmake before you configure and build 😃

[jnaidu360]

@deReeperJosh I feel like I'm almost there. Running "cmake .." finishes fine, but when I run the final command, "make -j $(sysctl -n hw.logicalcpu)", I get this output with an error partway through:
Output.zip

[deReeperJosh]

Not sure, doesn't look like that output shows any errors. Was there anything further up in the output log?

[deReeperJosh]

If it's easier, we can discuss this via discord (Grim_de#3727) so we don't crowd up the conversation here

[jnaidu360]

Got it. FR sent.

[jnaidu360]

I just fixed the issues. For Mac, I'm still thinking of a way to make the ingame button work, so in the meantime I disabled it for that OS. The Skylanders list is missing a bunch of characters right now, but everything else should be functional, I'm going to work on cleaning up the code and adding the missing characters.

[jnaidu360]

@mbc07 Thanks for checking. Just pushed the fixes.

[mbc07]

Very good, I have just two more requests, then this LGTM:

• See the scrollbar for the Skylander Slots? Could you move it inside the "Portal Slots" group box, so it looks like the "Game" and "Element" group boxes? On those group boxes, the group box itself is fixed and the scrollbar scroll its contents, in contrast to the current "Portal Slot" group box design, where it has a fixed height and the scrollbar scroll the entire group box.

• There's one last unnamed group box holding the bottom buttons. Please remove it and add those buttons directly to the parent QHBoxLayout.

---

Behavior wise, I tested the current design on both regular and High DPI screens, and they're working as expected. The crashes that were occurring when interacting with the "Enable Skylander Portal" checkbox are all gone too.

Code wise, the PR is failing to build on anything other than Windows, so this should be addressed before this PR can be merged. At the bottom of the PR discussion page, you can click on the "Details" link next to the builders that failed. This will direct you to our CI, where you can access the build log to see what failed and hopefully help pinpointing what went wrong...

[deReeperJosh]

Looks like you just need to include QVBoxLayout in the Window Header file

[jnaidu360]

@mbc07 @deReeperJosh Done and done. Check the new changes when you're free.

[mbc07]

Design wise, this LGTM now. Here's a screenshot of the current design, for reference:

Code wise, the build still is failing on anything but Windows, there's also a lint error now. Those should be fixed before the PR can be merged...

[jnaidu360]

Got it. Let's see if this works.

[jnaidu360]

Thanks for your help 😄

[JMC47]

Just did a quick little run through here and everything seems okay. Didn't really push it too hard, just wanted to make sure the functionality I use was still working.

[jnaidu360]

@AdmiralCurtiss Haha had to break out the Python to automate that enum class change. All fixed now. Thanks!

[jnaidu360]

Should build now.

[noahpistilli]

Also noticed you didn't fix the code that Admiral showed you on line 873.

[jnaidu360]

Also noticed you didn't fix the code that Admiral showed you on line 873.

There is actually an if statement to validate the input above the line, but I cleaned up the code anyways.

[jnaidu360]

@noahpistilli OK, should be all fixed.

[AdmiralCurtiss]

Only skimmed the current version but I didn't notice any obvious issues. Untested.

[jnaidu360]

Thank you both for the reviews :)