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

feat: Reimplement Jellyfish #895

Merged
merged 25 commits into from
Jan 10, 2024
Merged

feat: Reimplement Jellyfish #895

merged 25 commits into from
Jan 10, 2024

Conversation

nelson137
Copy link
Contributor

@nelson137 nelson137 commented Jan 5, 2024

Hi 👋 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

  • Item behaves like the musket:
    • It has a limited amount of ammo: 1 use
    • It reloads immediately when dropped
  • Players can only use the jellyfish while idle (i.e. can't be walking, falling, etc.)
  • The flappy jellyfish explodes if any of the following happen:
    • It goes outside the map
    • It collides with a player
    • The driver dies

Outstanding Questions

  • Is LoadedMap::is_out_of_bounds fine for explosion on out of bounds?
    • Is there a better way to detect this?
  • Should the explosion have an emote region?
    • e.g. the sword creates one 3.5x larger than the damage region
  • The item asset has 2 frames but I only used 1
    • Should it be animating while grabbed or on the ground?
  • Should the jellyfish go on the player's head while driving?
    • I tried but couldn't get the sprite to rotate

@nelson137 nelson137 changed the title Reimplement Jellyfish feat: Reimplement Jellyfish Jan 5, 2024
This does not explode the jellyfish, you get to use it again.
@nelson137
Copy link
Contributor Author

nelson137 commented Jan 6, 2024

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
Copy link
Member

zicklag commented Jan 7, 2024

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
Copy link
Contributor Author

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

@zicklag
Copy link
Member

zicklag commented Jan 7, 2024

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:

  • having the camera follow the jellyfish
    • I think the camera controller is hardcoded to follow players right now, but we could create a component that is used to set the camera follow properties for an entity, so that entities other than the player could tell the camera controller that it wants to be kept on the screen.
  • @erlend-sh 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". This isn't a problem with guns because they make an obvious clicking sound, so maybe that's all we need.
  • 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'll want an animation for retrieving the jellyfish so that it's clear what's happening when it disappears, and we could use it for when the jellyfish appears, too, probably with it's damage region delayed so that it doesn't do damage until it's animation finished when appearing.

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
Copy link
Member

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

Copy link
Member

@zicklag zicklag left a comment

Choose a reason for hiding this comment

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

@nelson137 I think we should probably implement the controls described above before merging, and I'm fine if we want to make try to make the hat wearable now, or do it in a follow-up, it's up to you.

Copy link
Member

@zicklag zicklag Jan 8, 2024

Choose a reason for hiding this comment

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

I just realized that I think the second frame of this is meant for use as a hat, and it's probably supposed to be kind of crooked on the fishes head. So we should be able to make it look right by updating the player attachment component with the proper offset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooh that makes sense. I thought the same and got it up on top of the player's head with a PlayerBodyAttachment but didn't think to move it over. I'll try that and see how good it looks.

@nelson137
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

zicklag commented Jan 10, 2024

Roger that, merging!

@zicklag zicklag added this pull request to the merge queue Jan 10, 2024
Merged via the queue into fishfolk:main with commit fb1e34a Jan 10, 2024
8 checks passed
@nelson137 nelson137 deleted the add-jellyfish branch January 10, 2024 01:42
@nelson137 nelson137 mentioned this pull request Jan 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Jan 12, 2024
Take 2 🎬 this is the second iteration of the Jellyfish item, based on
Duck Game's [RC
Controller](https://duckgame.fandom.com/wiki/RC_Controller).

More tweaks may come in follow-up PRs

## Notable Changes from [v1](#895)

- The camera follows any entity with the new `CameraSubject` component
- Entity must also have a `Transform` and `KinematicBody` to work
properly
- The flappy jellyfish collides with solids
- The flappy is now much more jerky and overly responsive to user input,
I may add acceleration to the left/right movement so that it floats
around more to give it a more natural feel
- Control the jellyfish like Duck Game's RC Controller
- The flappy is tied to the jellyfish hat, not the player
- If the driver dies/drops the jellyfish the flappy doesn't explode,
another player can pick it up and take control
- Despawn & dehydrate the jellyfish item when the flappy explodes

## ToDo

- ~Try to get the jellyfish on the player's head~ (will handle in
another PR for cosmetic changes)
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.

Re-implement Jellyfish
3 participants