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

Migrate Guava Predicates and Functions to Java implementations #6047

Open
wants to merge 70 commits into
base: master
Choose a base branch
from

Conversation

Jetz72
Copy link
Contributor

@Jetz72 Jetz72 commented Sep 3, 2024

Per #5717.

Not complete yet but wouldn't hurt to get some eyes on it while it's easier to revise things.

The switch-over of the implementation is done, but still need to finish cleaning up the helper methods. Many can be simplified through method references (e.g. ::equals and ::contains), but others don't have a good equivalent.

The dependency on Guava's Iterables is one point of uncertainty at the moment. It's too convenient to get rid of, but a handful of the filter methods still want Guava predicates. Could reimplement these manually in an IterableUtils class, or could just use the predicate::test hack to keep using the original library.

@Jetz72 Jetz72 requested a review from Hanmac September 3, 2024 14:34
@Jetz72
Copy link
Contributor Author

Jetz72 commented Sep 24, 2024

Added StreamUtils.random and converted a handful of uses of Aggregates.random to it instead. I didn't bother changing the ones that just passed the source collection right into the method, just the ones that had other operations that could also be part of a stream pipeline. I was gonna write up StreamUtils.max and min methods that took accessor functions like their Aggregate counterparts, but turns out you can just use stream.max(Comparator.comparing(whatever)).

Added some filter predicates that can be tacked on to CardDb.get[All/Unique]Cards to replace the non-promo and non-reprint variants of those methods.

When replacing Aggregates.random calls in the quest rewards, I realized I was cleaning up 5 copies of the same method with a different filter, so I ended up merging those into one plus some helper methods.

There are some lingering items that have been accumulated on my to-do list - e.g. individual methods that could be rewritten to take advantage of streams but aren't easy replacements, parameters or return types that could be shifted from Iterable to a more concrete collection, simplifying things elsewhere... But I'll probably just leave those alone because I have to draw the line somewhere.

The biggest thing I'll probably leave for another PR is a cleanup of CardLists.filter and its derivatives. Those methods create a new collection every time they're called, but often enough there isn't actually a need for one. There are many places where it's only used for counting items, picking a single item, or populating another collection (possibly with another call to CardLists.filter), and I wonder if the similar name to Iterables.filter has created the expectation that it behaves similarly.

That and whatever else I come across at the last minute aside, should be done soon. Need to remove the migration aid classes, resolve merge conflicts, cleanup imports, and spend some time testing it.

@Jetz72 Jetz72 marked this pull request as ready for review September 26, 2024 16:07
Jetz added 5 commits September 29, 2024 11:03
# Conflicts:
#	forge-game/src/main/java/forge/game/ForgeScript.java
# Conflicts:
#	forge-game/src/main/java/forge/game/phase/PhaseHandler.java
# Conflicts:
#	forge-ai/src/main/java/forge/ai/ComputerUtil.java
#	forge-core/src/main/java/forge/card/DeckHints.java
#	forge-game/src/main/java/forge/game/ability/AbilityFactory.java
#	forge-game/src/main/java/forge/game/cost/CostUntapType.java
#	forge-gui/src/main/java/forge/player/HumanCostDecision.java
@tool4ever
Copy link
Contributor

@Hanmac
do you still want to check anything? otherwise since release happened we might ship it
maybe keep it unsquashed bc of the size? 🤔

Jetz added 5 commits October 17, 2024 08:23
# Conflicts:
#	forge-gui-mobile/src/forge/adventure/data/AdventureEventData.java
Rewrite `generateParticipants`
# Conflicts:
#	forge-game/src/main/java/forge/game/GameEntityCounterTable.java
#	forge-game/src/main/java/forge/game/ability/effects/PumpEffect.java
#	forge-game/src/main/java/forge/game/staticability/StaticAbilityContinuous.java
@kevlahnota
Copy link
Contributor

@Hanmac is this PR good, I think java is more performant than guava, also do we need guava (I don't know if java covers all other collections guava have)

@Hanmac
Copy link
Contributor

Hanmac commented Oct 21, 2024

@kevlahnota i will look over this MR, but i think it should be good enough?

we can't really drop guava fully, i would miss my Table support

@Jetz72
Copy link
Contributor Author

Jetz72 commented Oct 21, 2024

I looked at cutting out more of Guava and recreating the functionality within other util classes, but the scope of that started expanding too quickly for this PR at least. They have a huge number of helper methods that we use, and many of those are implemented through internal helper classes which would either need to be reproduced as well or replaced with inline logic.

If we want to further lessen the dependency on Guava, I think a good direction would be expanding the functionality of FCollection, FCollectionView, and their derivatives. Passing data around through those instead of Iterables would reduce our reliance on Guava's Iterables methods.

For performance improvements, as mentioned above, I think CardLists could do with some scrutiny. I believe many methods there create entirely new collections each time they're called, and they're often used even when the call site only needs a filtered view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants