fishfolk / jumpy

Tactical 2D shooter in fishy pixels style. Made with Rust-lang 🦀 and Bevy 🪶
https://fishfolk.org/games/jumpy/
Other
1.64k stars 118 forks source link

feat: Reimplement Jellyfish #895

Closed nelson137 closed 8 months ago

nelson137 commented 8 months ago

Hi :wave: I've been wanting to contribute to jumpy for a bit so I started tinkering with the jellyfish item.. and it went more smoothly than I thought! I think I met all the original requirements but there were a few small things I wasn't certain about (see bottom).

Closes #577

Behavior

Outstanding Questions

nelson137 commented 8 months ago

Just pushed a feature I missed: pressing grab while driving despawns the flappy jellyfish. The owner "reclaims" that ammo and gets to use it again without having to drop it to reload.

zicklag commented 8 months ago

That's exciting! Thank you! I'm really glad to hear that it went smoothly, especially for an extra complicated item like the Jellyfish, and taking a swift look at the code it seems like you've done things the way I would have wanted, using a player state for driving the jellyfish.

Tomorrow I'm going to try to test it out and do a more thorough review and check out the questions.

Thanks again!

nelson137 commented 8 months ago

FYI I added it to dev level 1 on the platform above where you spawn

zicklag commented 8 months ago

I tested this out and it works! There are some polish things we'll want to add/tweak later probably, but we can do things incrementally, and I'm happy just merging this nearly as is.

Some thoughts on what we might want to tweak would be:

Regarding your questions:

Is LoadedMap::is_out_of_bounds fine for explosion on out of bounds?

Yeah, I think that's fine.

Should the explosion have an emote region?

I don't think it needs to in this case.

The item asset has 2 frames but I only used 1

I don't think it's supposed to move while on the floor, but I could be corrected on that.

Should the jellyfish go on the player's head while driving?

It seems like it should, but I think we need to have a different sprite that is non-rotated and designed as a hat frame that we can switch the item to. I'm still going to do a more careful code-review before merging, but otherwise I'm good doing whatever we need to in follow-up PRs.

erlend-sh commented 8 months ago

do you think the jellyfish should be dropped by the player automatically when it expires, so that it becomes clear that you can't use it again without picking it back up? It feels weird to me that once it's used there's no way to tell that it's used up and you have to drop and pick it back up to "reload".

It should be single-use without the reload. So when the jellyfish expires (blows up), just dissolve the jelly cap.

Should pressing the "fire" button while the jellyfish is already out explode the jellyfish or should it retrieve it to let you re-spawn it?

We should follow the same convention as the RC Controller from Duck Game which the jellyfish is based on:

The RC Controller controls a small TNT RC car when used. When the fire button is held, the controller controls the RC car with the usual movement controls. If the fire button is held and the throw/pickup button is pressed, the RC car will explode in a radius around its location, destroying itself in the process

nelson137 commented 8 months ago

That works for me! I'll start working on the new controls tomorrow or Tuesday then I'd like to get this merged; I'll do the rest in a separate one. I have another unrelated change I want to PR that's blocked on this 😅

nelson137 commented 8 months ago

Hm, the wiki doesn't specify what happens when you let go of the fire button while driving. @erlend-sh What is the desired behavior here? Unfortunately I don't own Duck Game

erlend-sh commented 8 months ago

The jellyfish should go static, and slowly descend to the nearest surface area.

I'm happy to have this merged in an experimental state so we can unblock you on further work. Since the jellyfish is a partially new design instead of a 1:1 copy of a Duck Game weapon we’re gonna have to try some stuff out anyway, so it’ll be a lot easier for me to provide further feedback once there’s a basic version to try out in-game.

nelson137 commented 8 months ago

That brings up another question that hasn't been mentioned yet -- should the jellyfish collide with solids on the map?

The jellyfish should go static, and slowly descend to the nearest surface area.

It sounds like the answer is yes. Currently, it collides with nothing but players. That should be straightforward enough to add, I've seen a few examples elsewhere in the codebase with the collision world.

To clarify, it also sounds like if the user holds fire again they regain control of the jellyfish wherever it is?

nelson137 commented 8 months ago

Like Erlend suggested I would like to merge this PR since there are so many changes. I will take care of everything we've discussed in a v2 PR, and possibly more depending on any tweaks that need to be made.

So far I have (1) the camera following anything with a new CameraSubject component and (2) the flappy jellyfish movement reimplemented using regular game physics (via KinematicBody) so it collides with solids.

I'm going to open a draft PR that I'll keep updated with my changes as I go.

zicklag commented 8 months ago

Roger that, merging!