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

Make CharacterContext usage clearer #466

Open
GregHib opened this issue Feb 26, 2024 · 5 comments
Open

Make CharacterContext usage clearer #466

GregHib opened this issue Feb 26, 2024 · 5 comments
Labels
enhancement New feature or request

Comments

@GregHib
Copy link
Owner

GregHib commented Feb 26, 2024

Not obvious that npc is for the source npc and not target npc

@GregHib GregHib added the enhancement New feature or request label Feb 26, 2024
@GregHib
Copy link
Owner Author

GregHib commented Jan 12, 2025

Could split npc and player events and for character subs cast or hope that they both cast to character fine

@GregHib
Copy link
Owner Author

GregHib commented Jan 14, 2025

Also might be worth considering switching to make the emitter the primary extension aka context(event) Player.() -> {} That would avoid the issue of needing a separate context, and has the benefit or tidying the scripts as player. everything get's removed.

Could have issues migrating though.

@GregHib
Copy link
Owner Author

GregHib commented Jan 16, 2025

Everything emitted by an entity, should have that entities context. Either the entity needs to be stored inside every event, or the entities context needs to be joined with the event.

Context receivers

Context receivers (not called context parameters) allow multiple contexts which works well in allowing both the emitter and the event to be in the same context. However removing player. from everything while shorter is more confusing as the information about the character you're working with is now implicit message() is not clearer because you don't know if that's sending a message to a player or an npc.

That mixed with the massive amount of migration required makes receivers a no-go.

Context splitting

Context splitting initially appeared more promising, splitting commands into Player and NPC context allowed only one or the other to be accessible depending on the type of handler. Issues first arrive when you want a context which has a specific type of source as well as target, so you need PlayerTargetNPCContext, PlayerTargetObjectContext, NPCTargetItemContext etc... lots of boiler plate.

This in and of itself isn't too bad however there's even more complexity when it comes to Actions.

Actions can't be split so easily because if you introduce a separate Player and NPC variant of queue's then functions that call queue's also must be split. This would really be a pain as a lot of combat (e.g. hit/damage) are generic to Character and so you'd have to duplicate all of those as well. On the other hand if you don't split Action, then you can't call specific functions inside of queues e.g. dialogues when the parent handler isn't a PlayerContext.

Either everything has to have a context i.e. add character to all events, which means object singletons can no longer be used, higher overhead, and it also makes the concept of EventDispatcher irrelevant as it's always required inside of events. Or everything that currently requires a context needs to depend indirectly on the entity instead.

@GregHib
Copy link
Owner Author

GregHib commented Jan 16, 2025

The best options seem to be pass the event as a param or embed the dispatcher in every event,

fun handler(block: suspend PlayerContext.(Event) -> Unit) {
}

Pros: Dev choice on whether to reference Event or decompose in lambda (if event is data class)
Cons: Not as tidy or easy to write subscribers

fun handler(block: suspend PlayerContext.() -> Unit) {
}

Pros: Can access values in a seamless clean way
Cons: More overhead from churning objects as all singletons will need to be changed to wrappers.

@GregHib
Copy link
Owner Author

GregHib commented Jan 17, 2025

Came up with a cleaner solution using generics, I realised extending the generic types allows you to have dynamic function calls, even better if those are inside the interface then you don't even need to import the extension functions.

interface Context<C : Character> {
    val character: C

    val Context<Player>.player: Player
        get() = character

    val Context<NPC>.npc: NPC
        get() = character
}

interface Target<C : Character, T : Entity> : Context<C> {
    val target: T
}

This should now enable adding the dispatcher to all events in a cleaner and more robust way. This will then enable the opportunity to remove the concept of Dispatcher altogether, possibly changing the call format from player.emit(Event(player, params)) to Events.emit(Event(player, params)) and simplify Events handlers from suspend Event.(EventDispatcher) -> Unit to suspend Event.() -> Unit.

Need to watch out for overlapping content with Interaction, Suspendable/cancellable events while looking into a solution for #209

@GregHib GregHib added this to Void Jan 17, 2025
@GregHib GregHib moved this to In progress in Void Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In progress
Development

No branches or pull requests

1 participant