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

Ambushed zone #80

Merged
merged 7 commits into from
Jan 15, 2024
Merged

Ambushed zone #80

merged 7 commits into from
Jan 15, 2024

Conversation

Cany0udance
Copy link
Contributor

First time ever doing a PR. Hopefully I didn't botch anything.

Known points of concern for the zone:

  • Sentry + Lagavulin may be too difficult for act 1, especially if the map layout forces the player into it
  • Occasionally, the shop may disrespect traditional shop rules. Instead of attack, attack, skill, skill, (power which my zone replaces with attack/skill) in the top row, it may become attack, skill, skill, skill, (other card) or skill, attack, skill, skill, (other card)
  • If the player has The Courier, newly generated cards come from the player's class as normal and not card draw cards as you would expect from my zone. Similar issue with potions

Copy link
Owner

@erasels erasels left a comment

Choose a reason for hiding this comment

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

Cool zone!

I'm unsure about the reward modification as a whole here though. Seeing only base game draw cards in the rewards seems very one note and only seeing draw potions aso doesn't feel great.

What about adding a single draw card as an additional card reward that players may pick in addition to the normal card reward?
https://github.com/erasels/SpireMapOverhaul/blob/main/src/main/java/spireMapOverhaul/rewards/SingleCardReward.java

That may also help justify the harder than normal fights.

}
}

private static class Locator extends SpireInsertLocator {
Copy link
Owner

Choose a reason for hiding this comment

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

Why is it not just a prefix patch? Or an rloc = 0? This seems unneeded


@SpireInsertPatch(locator = Locator.class)
public static void Insert(AbstractMonster __instance) {
if (AmbushedUtil.isInAmbushedZone() && AbstractDungeon.player.hasPower("Surrounded")) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't use string literals for ids. Use SurroundedPower.ID etc


@Override
public void replaceRooms(Random rng) {
replaceRoomsRandomly(rng, MonsterRoom::new, (room)->room instanceof EventRoom, 100);
Copy link
Owner

@erasels erasels Jan 12, 2024

Choose a reason for hiding this comment

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

You just want to replace all event rooms?

@Override
    public void replaceRooms(Random rng) {
        //Replace all event rooms with monster rooms
        for (MapRoomNode node : this.nodes) {
            if(node.room != null && EventRoom.class.equals(node.room.getClass())) {
                node.setRoom(new MonsterRoom());
            }
        }
    }

This is more effective


private void initCardDrawCardIDs() {
cardDrawCardIDs = new ArrayList<>();
cardDrawCardIDs.add("Pommel Strike");
Copy link
Owner

Choose a reason for hiding this comment

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

Please use the ID field every card has.

Also, if you want this to include modded characters you could ask ali for the code to dynamically check if a card is a draw card.

}

@Override
public void postCreateShopPotions(ShopScreen screen, ArrayList<StorePotion> potions) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this is not a good idea. Maybe just replace one potion with a draw potion. This seems overkill, and yoU're unlikely to want this many anyway. Also too much repetition in the potion choices

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would make the argument that due to the amount of zones there are and how small my zone is in terms of height + width, the odds of someone naturally wandering into a shop that happens to fall in my zone are very slim.

Also, replacing just one potion with a draw potion would be too petty to add as a modification I think. I am satisfied with the current setup, although maybe I could make it so one potion of each rarity is guaranteed to appear? So the player has the option to purchase a Swift Potion, a Gambler's Brew, and a Smoke Bomb (lol) / Snecko Oil. I do think three Swift Potions in one shop feels pretty bad.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I'd be okay with that.

}

@Override
public void modifyReward(RewardItem rewardItem) {
Copy link
Owner

Choose a reason for hiding this comment

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

Again, don't think this is a good idea since it's always the same potions.

new ZoneEncounter(AMBUSHED_BY_SENTRIES, 1, () -> new MonsterGroup(
new AbstractMonster[] {
new Sentry(-1000.0F, 0),
new Lagavulin(true)
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe use the combatmodifying zone to check for the laga encounters and nerf it a bit? This does seem a lot harder than the base game elites

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like the idea of having inconsistency in the game, where loading into a specific encounter nerfs a specific enemy. I'd rather try to find another enemy to replace Sentry with. I don't think the Sentry itself is that bad of an addition, it's the Dazes it adds that will REALLY mess a player up, especially if Laga debuffs them and their next turn is just full of Dazes.

@Cany0udance
Copy link
Contributor Author

So for the reward modification, here's how I see it.

The player is really looking for those high roll card draw cards that will help them throughout the rest of the game. We're talking Master of Strategy, Scrawl, Reboot, Acrobatics even. Therefore, I believe having 3 options per fight vastly improves how "good" the zone feels, as the odds of getting a high roll draw card are fairly reasonable.

If my zone only had 1 card draw choice per fight, you could see something like Prepared, then Impatience, then Escape Plan, then oops, you're out of the zone and you took all those hard combats for basically nothing. I think the current configuration is reasonable. As the zone only appears in acts 1 and 2, the zone gives you the chance to pick cards that will help your deck throughout the rest of the run, as card draw is always nice to have.

Also to answer all of your "why did you code it like this" questions, I do not know Java myself so I had ChatGPT program this whole thing. I will do my best to address some of your concerns and then update the PR.

@erasels
Copy link
Owner

erasels commented Jan 12, 2024

I see your point in regards to trying to get the highroll draw cards, however I believe that there's an issue with it. The thing you may be discounting here is that drawing cards is only good if you have things worth drawing. This zone can spawn in both act 1 and 2 where players are likely to be still building their deck and pick up value cards that help them win. You would not want to pick up an acrobatics when you have few high value cards or syngeristic effects.
While I agree that it's likely fine in act 2, in act 1 I find it hard to justify going into a hard zone to get draw cards when you don't even have a real deck to draw yet. That's a lot more risk than reward for a payoff that might require another act to payoff because you "skipped" ~3 combat prowess increasing card rewards.

Also, your code quality is fine, that locator just struck me as very weird. The others are just small fixes, I'm sure we'll get there.

}

@Override
public List<ZoneEncounter> getEliteEncounters() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The normal fights make sense to me since they're all ambush variants of existing normal fights. The surrounded mechanic means that you'll take somewhere around 25% more damage if you can't kill one enemy quickly, with the chance for more if you aren't able to turn around on a key turn. That can be deadly, but the zone is advertised as highly dangerous, so sure. I do think there's a risk that some of these fights can spike damage high enough in the first few turns that it's just not fun for the player, but that's something we can work out in playtesting (if it turns out to be an issue, we have options like giving the monsters weak on turn 1).

The elite encounters are where this might need some adjustment. I've made some fights like this (existing elites plus an extra enemy) as part of Corrupt the Spire, so while those didn't use the ambush mechanic, I have some idea of how difficult these will be.

For the act 1 fights, Louse and Nob seems close to the right balance. Having the Louse is like a mix of the burning elite strength and HP buffs (it's a bit more HP to work through and a bit more potential damage). This is more of an increase in difficulty relative to the original fight since it adds a new enemy (compared to the normal fights which don't add new enemies), but it's not that much harder.

Sentry and Lagavulin, on the other hand, is a huge amount of HP to get through. Even with Lagavulin sleeping at the start, if you can't kill the Sentry very quickly, the dazes make the long-term endurance fight against Lagavulin even harder. This seems like too much, even for what this zone is aiming for. There's a couple ways you could go instead. You could make a hard fight just by having Lagavulin be already awake and adding an small Acid Slime, or you could have a sleeping Lagavulin together with something that's less dangerous than a Sentry (a medium Acid or Spike Slime, perhaps).

For the act 2 fights, I get that they're the two Colosseum fights, but they don't match the difficulty I'd expect. Based on the act 1 elite fights for this zone, the idea seems to be that they're tuned to be more dangerous than normal elite fights in act 1 (since they add an extra enemy and surrounded). In contrast, the two slavers fight is less dangerous (I'd rather face the two slavers ambush than the normal three slavers elite fight). And while Taskmaster and Nob is harder to evaluate, it still feels like its in a weird place. Also, the two slavers fight doesn't have any enemies that count as elites, which is a bit too weird.

Replacing the two slavers fight with Book of Stabbing plus a weak enemy would give you one encounter that has around the right balance. I'm not sure what to suggest for the Taskmaster and Nob fight, though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The updated elite encounters make sense to me.

Having thought about it more, keeping Taskmaster and Nob seems fine. It can be a dangerous fight (especially if you aren't able to kill Nob quickly), but being dangerous is in line with the other elite encounters in this zone, and I don't think it's out of line with the other elite ambushes you've made. So let's give it a shot, and if it turns out to be too much, we can replace or change it.

One final thought on these encounters. Given that they're harder than normal, it may be worth considering at least a small additional reward. I get that if you're looking for card draw, getting a card reward with multiple card draw options is already a boost. But it might be worth something more on top of that, such as:

  • A small amount of extra gold
  • Having card rewards be more likely to be upgraded
  • An extra guaranteed potion

I don't think it's critical to the zone to have this, but right now my intuition is that the risk/reward of this zone may be slightly too skewed towards risk. Can't be sure of that without playing with it a bunch, but together with Gk's feedback, it supports the idea that a bit more reward may help balance things out. (Depending on what you go with, having normal encounters also get a small extra reward might make sense.)

case RARE:
// Randomly select between Smoke Bomb and Snecko Oil for Rare potions
if (AbstractDungeon.cardRandomRng.randomBoolean()) {
replacementPotion = PotionHelper.getPotion(SmokeBomb.POTION_ID);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting smoke bombs feels like a troll. Taking harder fights and having part of the "reward" be a near-useless potion isn't a great experience.

I'm not sure about the whole potion replacement thing either, but if you're doing it, it would land better if you cut the smoke bomb thing and just gave out Snecko Oils.

@Cany0udance
Copy link
Contributor Author

I made the following changes:

  • Sentry + Lagavulin is now Fungi Beast + Lagavulin. I still feel like this is slightly too difficult. I don't want to add a Louse because it would it's insignificant in the fight as a whole and the Nob encounter already has a Louse. I don't want to add a Slime either because 3 of my 5 normal act 1 encounters include a Slime which is already too much probably.
  • Red + Blue Slavers is now Byrd + Book of Stabbing. Again, I feel like this is slightly too difficult.
  • String literals nuked
  • Smoke Bombs removed from the zone entirely. I had them there for thematic purposes but losing them isn't that big of a deal
  • Shops will now always offer a Swift Potion, Gambler's Brew, and Snecko Oil. Though notably, the base price of these is ALWAYS the same and isn't picked within a range like regular shop potions are. Not sure how to fix that.

@erasels erasels merged commit 99fcae3 into erasels:main Jan 15, 2024
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.

3 participants