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

Add additional ray trace API #12162

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

TonytheMacaroni
Copy link
Contributor

@TonytheMacaroni TonytheMacaroni commented Feb 23, 2025

Supersedes #11166.

  • Adds overloads to World#rayTrace and World#rayTraceBlocks, as well as the method PositionedRayTraceConfigurationBuilder#blockCollisionMode, that take a BlockCollisionMode instead of the boolean ignorePassableBlocks to determine block collisions.
  • Add FluidCollisionMode.WATER.
  • Remove outdated javadocs on World#rayTrace, World#rayTraceBlocks, PositionedRayTraceConfigurationBuilder#fluidCollisionMode, and PositionedRayTraceConfigurationBuilder#ignorePassableBlocks that mention caveats with portal blocks and fluids that no longer exist. Portal blocks are always considered 'passable', and the value of ignorePassableBlocks has no effect on fluid collisions.

@TonytheMacaroni TonytheMacaroni requested a review from a team as a code owner February 23, 2025 00:07
/**
* Collide only with water.
*/
WATER,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going to expose water we should add lava to the nms enum and add it here aswell

/**
* Use the shape of a full block, but only consider blocks tagged with {@link org.bukkit.Tag#FALL_DAMAGE_RESETTING}.
*/
FALL_DAMAGE_RESETTING
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels kinda weird to expose this one, as i think it would feel pretty random to someone using the API. And its only really there for internal Falldamage calculations

Copy link
Member

Choose a reason for hiding this comment

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

yeah, you should be able to replicate it yourself with the predicate anyways right?

Copy link
Contributor Author

@TonytheMacaroni TonytheMacaroni Feb 26, 2025

Choose a reason for hiding this comment

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

Kinda - for some reason, this mode always uses a full block shape, instead of the actual collision shape of the block. I do agree that I don't see too much of a point of keeping it, beyond perhaps making replicating fall damage calculations in API easier to do. I don't see much of a point to not include it, though.

Copy link
Contributor

@notTamion notTamion Feb 26, 2025

Choose a reason for hiding this comment

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

It makes the enums we have in the API seem rather random and odly specific, same thing for the WATER, LAVA Review i left. If this uses a collision mode (full block shape) we don‘t otherwise have in the api then we should instead expose that one without the fall damage filtering part, that seems a lot more useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally am also on the side to expose a much as we need, as little as we can.

Overexposing internals for the sake of exposing them just makes your life harder when things change.

@Warriorrrr Warriorrrr added type: feature Request for a new Feature. scope: api labels Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: api type: feature Request for a new Feature.
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

5 participants