Add extra raytracing api#8205
Conversation
Machine-Maker
left a comment
Machine-Maker
left a comment
There was a problem hiding this comment.
Thanks for you contribution!
I like the idea of using a builder pattern here.
| + /** | ||
| + * Build and cast the raytrace checking only blocks. | ||
| + * | ||
| + * @param world the world to cast the raytrace in | ||
| + * @return the result | ||
| + */ | ||
| + @Nullable | ||
| + public RayTraceResult blocks(@NotNull World world) { | ||
| + Preconditions.checkArgument(world != null, "World cannot be null"); | ||
| + Location start = new Location(world, origin.getX(), origin.getY(), origin.getZ()); | ||
| + return world.rayTraceBlocks(start, direction, range, mode, ignorePassable, ignoreBlocks); | ||
| + } | ||
| + | ||
| + /** | ||
| + * Build and cast the raytrace checking both blocks and entities. | ||
| + * | ||
| + * @param world the world to cast the raytrace in | ||
| + * @return the result | ||
| + */ | ||
| + @Nullable | ||
| + public RayTraceResult entities(@NotNull World world) { | ||
| + Preconditions.checkArgument(world != null, "World cannot be null"); | ||
| + Location start = new Location(world, origin.getX(), origin.getY(), origin.getZ()); | ||
| + return world.rayTrace(start, direction, range, mode, ignorePassable, raySize, entityPredicate, ignoreBlocks); | ||
| + } |
There was a problem hiding this comment.
I think these methods should probably be on World, and just take a RayTraceQuery object.
Something to also do in the future (not in this PR) would be to make all the other ray trace methods utilize builders
There was a problem hiding this comment.
I think it's still useful to have utility methods to cast the raytrace directly from the builder.
If I add the methods with the RayTraceContext arg on World should I replace the new overload methods or simply add them. It would be rather odd to "lock" new api functionality behind the method with the context parameter. But on the other hand could be confusing to have multiple ways (and maybe implementations) to perform the same thing.
There was a problem hiding this comment.
I generally think that a "Builder" should have one job, to build some other object type. Now the Paper API isn't consistent on that, see the ParticleBuilder
EDIT: Also why wouldn't you just put those methods on the context type? What's the purpose of having them on the Builder specifically instead of the type that's being built.
There was a problem hiding this comment.
Likewise, you could argue the context object has one job, storing the raytrace configuration.
I think it's fair to assume that whenever one builds a context they also want to cast a raytrace using it. Those helper methods would simply combine the build and cast in one call.
There was a problem hiding this comment.
To be honest, i disagree; simply keeping data is not a responsibility: this turns objects into C structs. I don't see any problems of having a rayTrace method here.
PrimordialMoros
force-pushed
the
raytracing-api
branch
from
6e9dc00 to
6160287
Compare
PrimordialMoros
force-pushed
the
raytracing-api
branch
from
6160287 to
418b83a
Compare
| + * <p>Usage of the {@link Builder} is preferred over the super long | ||
| + * {@link World#rayTrace(Location, Vector, double, FluidCollisionMode, boolean, double, Predicate, Collection)} API. | ||
| + */ | ||
| +public class RayTraceContext { |
There was a problem hiding this comment.
It could but you'd need to repeat validation for all the arguments and also override some methods due to vectors being mutable. Not sure if it would be worth it.
There was a problem hiding this comment.
I'm curious on a couple things...
- I feel the max range is a bit too restrictive here, IMO if someone puts a huge number here too bad! I don't think it's great to limit it like this. Why was this range chosen?
- Why not use a predicate for ignoreBlocks instead? This way you could simply (in the predicate) define your own logic and possibly extend it in the future.
But in general, this makes that whole api MUCH nicer to handle. 😅
|
Superceeded by 23860da . A builder is something that might be useful, but in general, would probably be better off implemented in a different way (avoid passing World, etc) So if someone wants to pickup on that, PRs welcome. 😄 |
4 participants
New api allows to specify a set of blocks (or rather block positions) that will be ignored when performing a raytrace.
It also adds a builder to easily construct raytraces instead of using the world methods with a myriad args.
Should this be consolidated with previous raytrace patches?