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

[UI/UX] Reducing number of containers in the Pokédex #5400

Merged
merged 5 commits into from
Feb 25, 2025

Conversation

Wlowscha
Copy link
Contributor

@Wlowscha Wlowscha commented Feb 23, 2025

What are the changes the user will see?

Hopefully none.

Why am I making these changes?

Since the introduction of #5083 to main, players on certain devices and browsers have been experiencing crashes due to insufficient memory. While memory issues are pervasive in the Pokérogue implementation, the Pokédex exacerbated the problem by introducing a large number of new containers.

Specifically, the Pokédex followed the example of the Starter Select screen, which by itself creates a container for each starter (over 500). These containers already occupy a significant space in memory, and the Pokédex introduced new containers for each species (over 1000). What's worse, the containers are created in the setup() phase and never deleted.

Another PR, #5351, attempted to put a patch on these issues by creating the containers when the Pokédex or Starter Select are opened, and removing them afterwards. This only shifts the problem, while also introducing a lag of a few seconds when accessing or exiting these screens. This PR brings about more sweeping changes aimed at curbing the number of containers altogether.

What are the changes from a developer perspective?

Instead of creating a container for each species in the Pokédex, only 9x9=81 containers are created (the maximum number of Pokémon icons visible on screen at the same time). No additional lists of containers are created when filtering; instead, the relevant information about filtered Pokémon is stored in an array of interfaces. The user interface has been updated accordingly, in particular by reworking the logic for scrolling in the icon box.

This PR is self-consistent and reduces memory occupation due to the Pokédex specifically, which is the most urgent problem. Further improvements are possible with regards to the starter select screen, by cutting down on the number of containers in a similar way, or even reusing the Pokédex containers outright.

Whether the contributions from #5351 can still be beneficial after these changes remains to be seen.

Screenshots/Videos

Comparison from dev tools on Microsoft Edge, between current main (first screenshot) and with the changes from this PR (second screenshot), both from running the game locally for fair comparison. Notice that, as intended, the occupation from PokédexMonContainers is significantly less. In both cases, the screenshots are taken after starting the game, opening the Dex and closing it.
c81_without_after
c81_with_after

How to test the changes?

Just open the dex and fumble around. Since this is a pretty significant rework of the UI, it's very likely that bugs are hidden somewhere.

Checklist

  • I'm using beta as my base branch
  • There is no overlap with another PR? Some overlap with [Refactor][UI/UX] Reducing permanent memory occupation from Starter Select and Pokédex #5351
  • The PR is self-contained and cannot be split into smaller PRs?
  • Have I provided a clear explanation of the changes?
  • Have I tested the changes manually?
  • Are all unit tests still passing? (npm run test)
    • Have I created new automated tests (npm run create-test) or updated existing tests related to the PR's changes?

@Wlowscha Wlowscha requested a review from a team as a code owner February 23, 2025 12:25
Copy link
Collaborator

@DayKev DayKev left a comment

Choose a reason for hiding this comment

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

Just did a quick skim of the code for now.

@Madmadness65 Madmadness65 added UI/UX User interface/-experience related Performance Rewrites designed solely at improving existing performance labels Feb 23, 2025
Copy link
Collaborator

@DayKev DayKev left a comment

Choose a reason for hiding this comment

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

Pokedex still seems to work.

Copy link
Member

@SirzBenjie SirzBenjie left a comment

Choose a reason for hiding this comment

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

This is a good start. I hope this helps mobile users out.

Sorry @Wlowscha , I won't have my automated tests for the Pokedex here for this primarily because they are tied up with my testing infrastructure refactors (which relied on changing some stuff for game state that makes the tests not translate).

There's a lot more still to do, but doing this incrementally is wise

@SirzBenjie SirzBenjie merged commit 8cc5f65 into pagefaultgames:beta Feb 25, 2025
13 checks passed
Jimmybald1 pushed a commit to IBBCalc/Pathing-Tool that referenced this pull request Feb 27, 2025
…5400)

* PokedexMonContainer now has a method to change species.

* Not setting tint to 0 in the container

* Using only 81 containers in Pokédex

* Apply suggestions from code review

Co-authored-by: NightKev <[email protected]>

---------

Co-authored-by: NightKev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Rewrites designed solely at improving existing performance UI/UX User interface/-experience related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants