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: non-tile solids #903

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Conversation

nelson137
Copy link
Contributor

@nelson137 nelson137 commented Jan 16, 2024

Implement collisions for non-tile solids and a way to define them for elements in a map's YAML. This makes the CollisionWorld account for entities with the Solid component.

I started out trying to implement a door for #43 but quickly realized I couldn't make a closed door solid. This is its own PR because I want to make sure I'm going in the right direction here and discuss the implementation.

solids.mp4

Changes

Defining solids

Elements can be defined in each layer of a map's YAML file. Solids can now be defined on an element with a solid object.

Example element in a map layer:

  - pos: [630.0, 365.0]
    element: core:/elements/environment/demo_solid_box/demo_solid_box.element.yaml

Now, with a solid:

  - pos: [630.0, 365.0]
    element: core:/elements/environment/demo_solid_box/demo_solid_box.element.yaml
    solid:
      enabled: true
      pos: [630.0, 365.0]
      size: [16.0, 16.0]

The solid has its own position and size since a collider may need to be separate from the sprite. For example, the door sprite will need to be very wide for when it's open but the collider for a closed door will be very thin.

This was done by adding the new struct ElementSolidMeta to ElementSpawn. I wanted it to be a Maybe<ElementSolidMeta> but got an error from bones. My next best option was letting it be default-initialized for all elements but provide an enabled boolean that must be true for the solid to be created. Not ideal but it works.

Another downside is that the size of the solid collider has to be defined in the map, whereas the size of the KinematicBody is often defined in the element's .yaml data file and the size of the sprite is defined in the element's .atlast.yaml. Maybe this can eventually be moved into an element's asset configs.

Collision detection

Solids get a Collider component like kinematic bodies, which are also synced to the rapier colliders. This allows game code to easily control the collider. e.g. the door will want to disable the collider when it's open, which can be done by setting Collider.disabled to true.

I'm not convinced my changes to CollisionWorld::move_{horizontal,vertical} are correct, but this seems to mostly work.

Since the CollisionWorld::tile_collision{,_filtered} methods now detect any collision they should be renamed to CollisionWorld::collision{,_filtered}. If this is looking good so far I will make that change.

Game behavior

I added a few demo boxes to dev level 1 for testing. It works great from the testing I did except for a couple minor bugs. If you slide into the box to the left of where you spawn you stop short of the box. But if you walk up to the box you collide with it as expected. Sliding into the one in the far bottom right of the map behaves like you would expect. This is all shown in the video.

Additionally, critters cause a lot of collision warnings:

2024-01-16T22:02:52.902798Z WARN jumpy::core::physics: Collision test error resulting in physics body stuck in wall at Rect { min: Vec2(712.0, 96.1), max: Vec2(722.0, 106.1) }

Snails walk straight through solids, causing these warnings. Crabs may attempt and fail to surface under a solid after burrowing, also causing these warnings. I'm not sure what to do about this yet.

@zicklag
Copy link
Member

zicklag commented Jan 18, 2024

Cool, thanks for checking this out! I'm going to do a proper review of this as soon as I can, probably over the week-end if I can't find time at the end of the next couple days.

@nelson137
Copy link
Contributor Author

Great! No rush, I already added 1 thing I missed, and now I think I need to support multiple solids per element for the door 😅

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.

If you slide into the box to the left of where you spawn you stop short of the box.

That's actually a longstanding bug, not introduced here. I think it has something to do with how we set the position of the player when he "stands up" from sliding. I never figured out exactly why it happened or why it's different on the left than it is on the right side of the player.

If you could fix it that'd be a bonus, but that doesn't need to be done here. :)

src/core/elements/demo_solid_box.rs Outdated Show resolved Hide resolved
@nelson137
Copy link
Contributor Author

That's actually a longstanding bug, not introduced here

I can open an issue for that and then pick it up sometime after this

Instead we should put all of the metadata for the solids in whatever kind of elements that we want to spawn...

That makes a lot of sense, and I like that way better than putting solids in the map.

@nelson137
Copy link
Contributor Author

nelson137 commented Jan 20, 2024

I've finished making those changes.

I also realized that I was only using Collider for its rapier_handle and disabled. I decided to move all of this functionality into Solid, so they no longer need a Collider. I don't believe using a Collider for these kinds of entities is appropriate since it seems tailored for things that have more normal physics (gravity, friction, velocity, interaction with jump-through platforms like wood, etc.).

Another option would've been to go full KinematicBody + Collider and make Solid just a marker component with no data. But IMO solids are fundamentally different kinds of entities whose only overlap with other phsyics objects is collision detection, and therefore they merit their own special treatment. Let me know if you disagree / have any thoughts on this.

This change means that a Solid has full control over the rapier collider, making it a nice interface to update the size, position, and disabled of the collider in-game. I updated the demo to show this.

Anyway, if everything looks good I can rebase onto master and drop those DELETEME commits.

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.

This looks good, other than hiding the collision shapes.

There is one other but I noticed, but I don't know how to reproduce. When I started the game the first time, for some reason the collision shape of the demo box on the right started moving and ended up stretched out like the red annotated box in this screenshot:

image

And it was moving, but not the sprite, just the collision shape that you could see because the debug lines! I'm not sure what would cause that. I restarted the match and it didn't happen again, but it's something to keep an eye out for I guess if it happens later or we can figure out what causes it.

impl Default for DebugSettings {
fn default() -> Self {
Self {
show_kinematic_colliders: true,
Copy link
Member

Choose a reason for hiding this comment

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

This should be set back to false before merging.

@nelson137
Copy link
Contributor Author

And it was moving, but not the sprite, just the collision shape

Sorry, my last message wasn't very clear. I did that on purpose to show that changing the properties of Solid syncs with the collider. So for that one box I change the position, size, and toggle disabled every second.

And the change to DebugSettings should be in one of the DELETEME commits, and I will drop those before rebasing

@zicklag
Copy link
Member

zicklag commented Jan 25, 2024

Oh, OK, you're right, I forgot about that in your comment. Cool, that's actually great! So then I'm all good with this, you can drop those commits and I'll merge it!

Comment on lines 91 to 94
let solid = solids.get_mut(**solid_ent).unwrap();
solid.pos.x += 0.05;
solid.size.x += 0.03;
solid.disabled = time.elapsed_seconds() as u32 % 2 == 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This controls the collider of that one box

@nelson137 nelson137 force-pushed the feat/non-tile-solids branch from d242eb2 to 2b44e21 Compare January 25, 2024 18:07
This allows for easier control of the rapier collider. Game code can
disable the collider and it will be synced to the rapier collider.
This allows the game to update the size of a `Solid` and for it to take
effect in the rapier physics simulation.
We were only using `Collider` for it's disabled and rapier handle
fields. Meanwhile, `Solid` contains the size and position for the solid.

This commit adds fields to `Solid` so that it can be used exclusively,
and solid entities no longer need a `Collider.
@nelson137 nelson137 force-pushed the feat/non-tile-solids branch from 2b44e21 to 33b10ab Compare January 25, 2024 18:11
@zicklag zicklag enabled auto-merge January 25, 2024 18:13
@zicklag zicklag added this pull request to the merge queue Jan 25, 2024
Merged via the queue into fishfolk:main with commit 461b21a Jan 25, 2024
8 checks passed
@nelson137 nelson137 deleted the feat/non-tile-solids branch January 25, 2024 18:18
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.

2 participants