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

Signal Channels #206

Draft
wants to merge 1 commit into
base: 1.19
Choose a base branch
from
Draft

Conversation

Cypher121
Copy link
Contributor

Provides an API for publishing and subscribing to events. Supports positional (origin+range, box), world, and global events, and positional, (block)entity-bound, and worldwide/global listeners.

Current implementation is a Proof-of-Concept and has no global listener support yet, as the listener subscriptions need to be heavily re-organized (either per-chunk storage or a data structure with quick access for neighboring coordinates).

The test mod has a regeneration block that emits events within 4 blocks of itself. Events are caught by player listeners and apply Regen I.

@SilverAndro
Copy link
Contributor

Im a bit confused/concerned about the passing of Codec<T> everywhere, what problem is this trying to solve?

Named channels seems to already have an ID, so there must be some AoT communication about what data to expect, right?

Copy link
Contributor

@SilverAndro SilverAndro left a comment

Choose a reason for hiding this comment

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

Also this minor thing


public List<SubscriptionImpl.Named<?>> getNamedSubscriptions(Identifier id) {
cleanSubscriptionMaps();
return namedSubscriptions.computeIfAbsent(id, k -> new ArrayList<>()); //TODO change raw gets to computes!
Copy link
Contributor

@SilverAndro SilverAndro Oct 24, 2022

Choose a reason for hiding this comment

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

//TODO change raw gets to computes!

Is this outdated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is, yeah

@Cypher121
Copy link
Contributor Author

Every instance of the named channel given by SignalChannel.getOrCreateNamed gets its own codec but can share the ID with others. ID determines who gets it, and it's then re-encoded from the source codec to the target.

The intent of named channels is to be a way of communication between mods without a dependency. Some lightweight compat may not call for an optional dependency with all its pitfalls, so instead a named channel with the same ID as the other mod is created and subscribed to. The original type is not available, so a new one with a compatible Codec is created and used as the message type itself.

The conversion (kinda scuffed, it definitely should cache both codec steps) is in SubscriptionImpl.Named I believe.

@lukebemish
Copy link
Contributor

How often is that codec based conversion likely to fire, in practice?

@Cypher121
Copy link
Contributor Author

For each event fired, once for every unique subscribing codec, plus possibly an LRU cache on top for repeated identical events. So realistically once or twice per event at most? It's also not likely to be a big object to serialize to begin with.

Outside of better caching, the part I'm most unhappy about with this one is converting it through JSON, which is a lossy format.

@EnnuiL EnnuiL added new: module A pull request which adds a new module t: new api This adds a new API. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. library: misc Related to the misc library. s: wip This pull request is being worked on. and removed s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. labels Oct 26, 2022
@@ -0,0 +1,3 @@
accessWidener v2 named

transitive-accessible class net/minecraft/block/entity/BlockEntityType$BlockEntityFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this used for? I dont see this class referenced anywhere in your code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implicitly converted to from a constructor reference in the testmod when creating a BlockEntityType with the builder. Had to be copied from the block_entity module due to transitive AWs not working

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, alright

Is it possible to keep it in the testmod resources or does loom throw a fit with that?

@SilverAndro
Copy link
Contributor

SilverAndro commented Oct 31, 2022

Taking a bit of a deeper look, it doesnt seem like this currently support switching the bind at all? I can think of a few usages for that (i.e something that switches between block and entity form?) but you can cancel and and create a new subscription so I dont think its too major

As for the storage system, iirc minecraft itself already has something similar that could be reused? theres an EntityLookup<T extends EntityLike> that might be possible to build around?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
library: misc Related to the misc library. new: module A pull request which adds a new module s: wip This pull request is being worked on. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants