casey-c / FilterTheSpire

Customize your Slay the Spire experience
MIT License
4 stars 3 forks source link

Make RelicRngSimulator more generic for all relic types #8

Closed beeyee0401 closed 3 years ago

beeyee0401 commented 3 years ago

It seems like none of the Simulators need to instantiate a class for what they're doing. Wondering if it's better if we make them all Static. I've done the RelicRngSimulator as an example.

There are a couple reasons for this:

casey-c commented 3 years ago

I like the idea of making all the simulators static and agree that none of them need an actual object to do what they need to do. I think it's worth holding out on merging this one PR for one performance concern: we probably shouldn't use a HashMap here to get the number of times we need to skip over an RNG step. The (minor) performance loss caused by the HashMap not being super cache friendly compared to something like a simple array accessed by the relicTier.ordinal() will start to add up when we take into account needing billions of simulation attempts.

beeyee0401 commented 3 years ago

I didn't know .ordinal() was a thing. That seems really awkward though. So our array would be something like below.

[null, null, 2, 0, 1, null, 4, 3]

private static final HashMap<AbstractRelic.RelicTier, Integer> RelicRngMapping = new HashMap<AbstractRelic.RelicTier, Integer>(){{
        put(AbstractRelic.RelicTier.UNCOMMON, 0);
        put(AbstractRelic.RelicTier.RARE, 1);
        put(AbstractRelic.RelicTier.COMMON, 2);
        put(AbstractRelic.RelicTier.SHOP, 3);
        put(AbstractRelic.RelicTier.BOSS, 4);
}};

public static enum RelicTier {
        DEPRECATED,
        STARTER,
        COMMON,
        UNCOMMON,
        RARE,
        SPECIAL,
        BOSS,
        SHOP;

        private RelicTier() {
        }
    }

I think I would rather just have constant integers declared and passed to RelicRngSimulator.getRelicPool(). Reading up on it, it seems like using Ordinal shouldn't really be a thing too

casey-c commented 3 years ago

Agreed with your assessment on the ordinal() not quite working here. Merged the changes finally since the performance concern has been managed