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

Interface for objects that have a world associated with them #7234

Closed
vytskalt opened this issue Dec 31, 2021 · 16 comments
Closed

Interface for objects that have a world associated with them #7234

vytskalt opened this issue Dec 31, 2021 · 16 comments
Labels
type: feature Request for a new Feature.

Comments

@vytskalt
Copy link
Contributor

vytskalt commented Dec 31, 2021

Is your feature request related to a problem?

In my case I need to get the World of an Event (for per-world event listeners). Without this interface I'd need to do something like this:

public static @Nullable World getWorld(Event event) {
        if (event instanceof WorldEvent worldEvent) {
            return worldEvent.getWorld();
        } else if (event instanceof PlayerEvent playerEvent) {
            return playerEvent.getPlayer().getWorld();
        } else if (event instanceof EntityEvent entityEvent) {
            return entityEvent.getEntity().getWorld();
        } else if (event instanceof InventoryEvent inventoryEvent) {
            Location location = inventoryEvent.getInventory().getLocation();
            return location == null ? null : location.getWorld();
        } else if (event instanceof InventoryMoveItemEvent moveItemEvent) {
            Location location = moveItemEvent.getInitiator().getLocation();
            return location == null ? null : location.getWorld();
        }

        // And so on...
}

Describe the solution you'd like.

Add an interface with the method getWorld which would be implemented for all objects that might have a World associated with them (for example Chunk, Block, Location, Entity, events etc.)

Describe alternatives you've considered.

See above

Other

No response

EDIT: I've found a patch for 1.8.8 which adds this exact interface: https://github.com/Electroid/SportPaper/blob/master/patches/api/0032-Physical-interface.patch

@vytskalt vytskalt added status: needs triage type: feature Request for a new Feature. labels Dec 31, 2021
@MiniDigger
Copy link
Member

mmmh, not sure I like the name, but the API itself sounds cool

@Machine-Maker
Copy link
Member

what about WorldIdentifiable, WorldLike, WorldIdentified

@Owen1212055
Copy link
Member

Isn’t this supposed to be covered by adventures pointer system?

@vytskalt
Copy link
Contributor Author

vytskalt commented Jan 3, 2022

Isn’t this supposed to be covered by adventures pointer system?

I think for simplicity the interface should be used instead.

@vytskalt
Copy link
Contributor Author

vytskalt commented Jan 10, 2022

I have made the patch, just need an agreement on the name of the interface. Also, I don't know which package to put the interface in.

@Owen1212055
Copy link
Member

Although an interface like this is simple, it's nice to have it use this system have it integrated into a variety of places. This will allow it to be much easier if new types were added, for example locations, or uuids. It's just an example, but there is no reason to limit it just for the sake of simplicity, especially since there is so many little values that can be shared by lots of objects.... like entities, locations, chunks, and etc. That's why adventure's pointers system was implemented, and I would think would be neat to make use of this.

@vytskalt
Copy link
Contributor Author

vytskalt commented Jan 11, 2022

Although an interface like this is simple, it's nice to have it use this system have it integrated into a variety of places. This will allow it to be much easier if new types were added, for example locations, or uuids. It's just an example, but there is no reason to limit it just for the sake of simplicity, especially since there is so many little values that can be shared by lots of objects.... like entities, locations, chunks, and etc. That's why adventure's pointers system was implemented, and I would think would be neat to make use of this.

How would that be implemented though? Allocating a Pointers for every object (events too) that can have a world (or anything else) would probably not be very efficient since it uses a HashMap internally.

@Owen1212055
Copy link
Member

Owen1212055 commented Jan 11, 2022

The performance implications of a map call isn't really that bad. Sometimes the api needs to sacrifice some small nanosecond differences in order to have a robust api.

I don't think it's large enough to warrant a performance implication.

Additionally, you don't really even need to use that, see some example implementations:

#6445

Example similar to this:

#5798

Edit: it seems I didn't realize that there was a new Pointers part of this.

See: https://github.com/PaperMC/Paper/pull/6634/files

@Machine-Maker
Copy link
Member

Machine-Maker commented Jan 11, 2022

So I know one plus of an interface, is there can be more than just the 1 method to get the World from whatever, in any cases where it might be useful to have multiple methods on one interface. kind of hard to replicate that with pointers. Like if on the world interface, we wanted to have getWorldName() and getWorldKey() or something like that. An interface affords more flexibility

@kezz
Copy link
Contributor

kezz commented Jan 11, 2022

The performance implications of a map call isn't really that bad.

fwiw, there's an adventure pr that will remove the need for a map backing per object

@Rincewind34
Copy link

Rincewind34 commented Jan 11, 2022

I think the Pointer System in itself does definitely have some good use cases, but this ain't one of them. The design goal looks to be to provide some kind of maximal abstraction what is not requested by this issue.

And considering the performance impact: I think it goes far beyond just ignoring a look up in a hash map. Looking at an event that might be fired a lot (like PlayerMoveEvent) I wouldn't want to through every concern about performance out the window. Using the interface allows Java to make way more optimizations while the Pointer system is so abstract, that it doesn't allow for that much to be optimized.

I think some kind of middle ground would be to do both.

@Rincewind34
Copy link

Rincewind34 commented Jan 11, 2022

Also I just wanted to raise thoughts about adding that to the class "InventoryEvent". To me that seems counter intuitive at first.
After some thinking about the semantics: what we are essentially doing (at least this is how it feels to me) is e.g. changing the super class from PlayerEvent to WorldEvent. And I think doing that would make sense since every event concerning a player or an entity is automatically taking place in a world and is directly affecting that specific world. I'm hassling on applying that same logic for an inventory event since not every inventory event is affecting a world.

I'm perfectly aware that actually changing the super class is not a good option since it might collide with proper backwards compatibility, so an additional interface would be the next best solution from a syntactical standpoint

@electronicboy
Copy link
Member

Being perfectly honest, I'm poised towards closing this

as with every feature request, etc, we have to determine if something is even remotely maintainable, and this is far too much of a noisy patch to do so, this is basically asking for us to have a patch file modifying hundreds of classes in the quest of adding an interface, a patch which would be far too noisy for me to say would be even remotely maintainable

All of that, for a single interface and a method, is honestly far from justifiable. post hard fork, this would potentially be an option, but even there it feels kinda shoddy, changing event hierarchy is also not a viable option given that this has many wider implications on codebases

@kennytv
Copy link
Member

kennytv commented Jan 12, 2022

See above; closing due to patch effort involved for such a small effect, use for this specifically is arguably low, and it'd open a discussion about what else could have a one-method interface vs. just one for worlds

@kennytv kennytv closed this as completed Jan 12, 2022
@vytskalt
Copy link
Contributor Author

See above; closing due to patch effort involved for such a small effect, use for this specifically is arguably low, and it'd open a discussion about what else could have a one-method interface vs. just one for worlds

My use case was per world event listeners (for minigames). I guess I'll have to use that ugly method that I provided in the issue message...

@electronicboy
Copy link
Member

easy to just add a single thing at the top of every method to get the world and check, far too much of a diff for something which is so trivially solvable using a slightly different code pattern, you're still gonna have to handle the event itself and it's peculiarities somewhere, I don't see how World world = event.getX().getWorld() is that big of a deal to pop at the top of the method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Request for a new Feature.
Projects
None yet
Development

No branches or pull requests

9 participants