Fixed Bouncer possibly rejecting Explosive Bunny critter releases

[drunderscore]

Fixes #2616

When using a Bunny Cannon, an Explosive Bunny item (which is also a critter release item) is used to create an Explosive Bunny projectile, which will later (in the future) release an Explosive Bunny NPC, by the release critter packet. The existing checks required that the player be actively selecting the item to create the critter, however this didn't make sense for Explosive Bunnies, as they would be released in the future, possibly when the player was no longer selecting that item.

This commit relaxes the restrictions on Explosive Bunny critter releases, now requiring either holding the release item, or having recently created an Explosive Bunny projectile, and that the release coordinates are within the area of one of their Explosive Bunny projectiles.

[tru321]

i guess the old code should be on after checking the type, or it will never executed (not tested yet)

if (type == NPCID.ExplosiveBunny){ //new code }else{ //old code }

[drunderscore]

@tru321 Not exactly... your issue is valid but solution isn't correct, as we still want to check if you're holding the Explosive Bunny item, as it can also be used to release the critter. I've updated the commit to ensure the correct behavior is maintained (reject if the item isn't being held for everything other than an Explosive Bunny)

[tru321]

i've test the code and notice some:

change the "&&" to "||" because crafting packet can be one of either "type" or "style". the old line detect only if both crafted. if (args.Player.TPlayer.lastVisualizedSelectedItem.makeNPC != type || args.Player.TPlayer.lastVisualizedSelectedItem.placeStyle != style)

add style detection here because the style from explosive bunny projectile is 1 or 2. to make sure its from item or projectile. if (type == NPCID.ExplosiveBunny && (style == 1 || style == 2)) { explosives bunny projectile detector code: i'm stuck here because after debug the countOfExplosiveBunnyProjectilesInRange, sometimes its detect the projectile and sometimes not }

[punchready]

Basically with the current code (and with the old code, actually) one can simply hold an item with placeStyle = 0, send an npc release with any npc id and style set to 0, and if ([...].makeNPC != type && [...].placeStyle != style) will fail because the style matches, allowing the packet through entirely. It makes more sense to first check if the npc is an explosive bunny with valid style of 1 or 2 and a valid projectile is active and early exit in that case:

if(isExplosiveBunny && styleValid && projectileExists) {
    return;
}
if(isInvalid) {
    //reject
}

For performance it also does not make sense to first count all valid projectiles and then compare the count to 0, better to implement a manual loop to allow exiting early to go from O(n) to O(log n) at least.

[drunderscore]

Fixed existing code to check makeNPC and placeStyle separately, and now try to find a projectile within range using Any, instead of doing whatever dumb thing I was doing before by Whereing and Counting.

[punchready]

It is probably still necessary to limit the style to 1 or 2 for bunny projectiles. And perhaps there is a return missing after the last rejectForCritterNotReleasedFromItem? I'd still move the projectile check as an optional exemption above the other logic, but the current code gets the job done too 👍

[Arthri]

i've test the code and notice some:

change the "&&" to "||" because crafting packet can be one of either "type" or "style". the old line detect only if both crafted. if (args.Player.TPlayer.lastVisualizedSelectedItem.makeNPC != type || args.Player.TPlayer.lastVisualizedSelectedItem.placeStyle != style)

add style detection here because the style from explosive bunny projectile is 1 or 2. to make sure its from item or projectile. if (type == NPCID.ExplosiveBunny && (style == 1 || style == 2)) { explosives bunny projectile detector code: i'm stuck here because after debug the countOfExplosiveBunnyProjectilesInRange, sometimes its detect the projectile and sometimes not }

I think you're able to submit a review, try that next time since code suggestions are easier to visualize

[Arthri]

This works, but if the bunny is shot far enough, you still get kicked

[drunderscore]

@Arthri Yeah, I suppose I could iterate all this player's projectiles looking for the a Bunny, instead of just the recent ones... I just threw this together so might redo it anyway.

[Arthri]

I'm not sure how recent projectiles work but, I fired a bunny first thing and it still kicked me

[punchready]

RecentlyCreatedProjectiles only keeps projectiles for 5 seconds, so things go 💥 if your bunny was in the air for more than that

[tru321]

@Arthri Yeah, I suppose I could iterate all this player's projectiles looking for the a Bunny, instead of just the recent ones... I just threw this together so might redo it anyway.

i tried to inspect projectile directly on Main.projectile, its work like a charm. lock (Main.projectile) { areAnyBunnyProjectilesInRange = Main.projectile.Any(projectile => { return projectile.type == ProjectileID.ExplosiveBunny && projectile.owner == args.Player.Index && projectile.active && projectile.WithinRange(new Vector2(x, y), 32.0f); }); }

i'm testing it with packet crafting. its work too as it should be.

[punchready]

This still allows mass spawning of bunnies with only one projectile active

[tru321]

I think mass spawning goes on different scope. Its hard to tell from what bunny projectile that bunny spawn.

Marking a bunny projectile on projectile update like give them tag like AlreadySpawnedABunny=false then set them true in this packet it might do, but i guess that goes too far :)

[hakusaro]

@Kojirremer did you get a chance to test this fix or do you need a rebuild?

[Kojirremer]

@Kojirremer did you get a chance to test this fix or do you need a rebuild?

I didn't since at the time it still looked like a WIP, and then I eventually forgot about it.