Add setting for Illusioners in raids. - #1819
Conversation
kacimiamine
left a comment
There was a problem hiding this comment.
While I can't look into the code right now to check the implementation, you can for now include // Purpur comments and also I don't see why we need a variable there ?
| private Raid.RaidStatus status; | ||
| private int celebrationTicks; | ||
| private Optional<BlockPos> waveSpawnPos = Optional.empty(); | ||
| + public boolean includeIllusioners; |
There was a problem hiding this comment.
Why the need of a variable when you can just use the config directly ? This is unnecessary
| } else if (groupNumber >= this.getNumGroups(Difficulty.HARD)) { | ||
| if (ravagersSpawned == 0) { | ||
| - ridingRaider = EntityTypes.EVOKER.create(level, EntitySpawnReason.EVENT); | ||
| + if (level.purpurConfig.illusionerSpawnInRaids) { |
There was a problem hiding this comment.
Diff like this should start with // Purpur start comment and end with // Purpur end
| + : EntityTypes.ILLUSIONER.create(level, EntitySpawnReason.EVENT); | ||
| + } else { | ||
| + ridingRaider = EntityTypes.EVOKER.create(level, EntitySpawnReason.EVENT); | ||
| + } |
There was a problem hiding this comment.
// Purpur end should be after this
| int bonusSpawns; | ||
| switch (type) { | ||
| case VINDICATOR: | ||
| + case ILLUSIONER: |
| VINDICATOR(EntityTypes.VINDICATOR, new int[]{0, 0, 2, 0, 1, 4, 2, 5}), | ||
| EVOKER(EntityTypes.EVOKER, new int[]{0, 0, 0, 0, 0, 1, 1, 2}), | ||
| PILLAGER(EntityTypes.PILLAGER, new int[]{0, 4, 3, 3, 4, 4, 4, 2}), | ||
| + ILLUSIONER(EntityTypes.ILLUSIONER, new int[]{0, 0, 1, 0, 1, 1, 0, 1}), |
|
As a side note, I recommend you check the CONTRIBUTING file and granny's comment on the last PR, as it can help you in future contributions (You can also check other merged PRs how they are done) |
|
I've added the missing comments. I created the "includeIllusioners" variable as a workaround since the original approach kept giving me errors. The code still works as intended with the variable, though. |
What errors? This variable is 100% unnecessary. We should look into those issues instead of working around them like that. |
|
I just applied your patch and removed that variable and I got no errors. As Billy said, what kind of issues are you encountering? And those comments are confusing, they should describe exactly the feature as stated in here e.g. // Purpur - Ability for illusioners to spawn during raids |
|
Turns out it was an issue with the cache. Derp. Do you want "// Purpur - Ability for Illusioners to spawn during raids" to be in all spots for the Illusioner code? |
|
It looks you sent the patch to your other PR instead of this. Why are you using the GitHub UI instead of git commands? I really suggest you join the discord and talk there about features and issues you might encounter, as these feel like redundant reviews across PRs which we're happy to help you on how to maintain better code |
|
I use the UI because it's what I'm used to. I'm already in the Discord too. I have the same username: Mickey42302. I guess I'll be more transparent about the next idea I'm planning to work on. This one will probably require more complex code. |
|
Understanding the basics of git and its commands will simplify a lot of work, especially when handling such PRs e.g. instead of uploading one file per commit, you can add all of them in one meaningful commit, anyway a bit off-topic. And yes, best discussing ideas in discord and especially if you feel the implementation is complex or the comments look confusing. There will people willing to help on that, so better talk therefore any issues you have (even this one you can share it there), this should avoid the redundant reviews and make it easier |
BillyGalbreath
left a comment
There was a problem hiding this comment.
everything looks good as-is. this is just my personal nit-pick. feel free to disregard if you want :)
| Raider ridingRaider = null; | ||
| if (groupNumber == this.getNumGroups(Difficulty.NORMAL)) { | ||
| ridingRaider = EntityTypes.PILLAGER.create(level, EntitySpawnReason.EVENT); | ||
| + // Purpur start - use the setting to determine if Ravagers with Illusioners should spawn |
There was a problem hiding this comment.
it looks like this should be moved two lines down
| boolean isBonusGroup = this.shouldSpawnBonusGroup(); | ||
|
|
||
| for (Raid.RaiderType raiderType : Raid.RaiderType.VALUES) { | ||
| + // Purpur start - exclude Illusioners if the feature is disabled |
There was a problem hiding this comment.
you can collapse all this into a single line to minimize the diff
| - ridingRaider = EntityTypes.EVOKER.create(level, EntitySpawnReason.EVENT); | ||
| + if (level.purpurConfig.illusionerSpawnInRaids) { | ||
| + ridingRaider = random.nextBoolean() | ||
| + ? EntityTypes.EVOKER.create(level, EntitySpawnReason.EVENT) |
There was a problem hiding this comment.
idk how i feel about a ternary operator spreading across multiple lines. if it's for readability might as well just use a standard if/else with proper brackets.
+ if (level.purpurConfig.illusionerSpawnInRaids) {
+ if (random.nextBoolean()) {
+ ridingRaider = EntityTypes.ILLUSIONER.create(level, EntitySpawnReason.EVENT);
+ } else {
+ ridingRaider = EntityTypes.EVOKER.create(level, EntitySpawnReason.EVENT);
+ }
+ } else {
+ ridingRaider = EntityTypes.EVOKER.create(level, EntitySpawnReason.EVENT);
+ }but that still looks very busy, especially with repeat code.
personally, i would shorten the code a little more by combining the two conditions required for the illusioner to spawn. here's all those lines merged into a one-liner that is easier to read, imo.
ridingRaider = (level.purpurConfig.illusionerSpawnInRaids && random.nextBoolean() ? EntityTypes.ILLUSIONER : EntityTypes.EVOKER).create(level, EntitySpawnReason.EVENT); // Purpur|
The changes are applied now. |
I'd like to suggest adding a "spawn-in-raids" setting for Illusioners. Having these mobs as a part of raids would add a nice challenge.
By default, the feature is disabled. However, players can opt in by changing it to "true" in the "purpur.yml" file.