Carpet

Carpet

2M Downloads

Extension's rules aren't syncronized with clients

altrisi opened this issue ยท 7 comments

commented

Currently, ServerNetworkHandler only sends rules from Carpet's SettingsManager.

CarpetServer.settingsManager.getRules().forEach(data::withRule);

This basically means that rules from other SettingsMangers will never* be received by clients from remote servers.

There are various ways to solve this:

  • Send all rules to clients by appending them with the current withRule method and then loop through all SettingsManager instances in the client (probably performance intensive in the client if extensions have many rules/many custom managers, but would keep compatibility)
  • Change the protocol so it also sends the SettingsManager identifier with each rule (or another way to identify it) and find SettingsManager by that identifier in the client. May, however, probably break when connecting old clients/servers (would it?).

Maybe a good way would be to add the latest to the 1.17 branch (if it breaks), since most versions are basically incompatible with the previous. May provide a PR at some point

* Technically, those are sent in the updateRuleWithConnectedClients method, since it is called from anySettingsManager, but those aren't properly added in the client since it only checks if that rule exist in Carpet's.

commented

I'll suggest to keep compatibility. There are people who use client to connect to servers in different minecraft versions / carpet versions by mods or plugins or whatever things. If it's not backwards compatible it will restrict the client carpet version that people can use.

Performance issue should be fine since even rule synchronization is not something that will happen quite often.

commented

Ah wait, why would extension have its own SettingsManager. Extension only need to let carpet's setttingManager to parse its own setting class, which will add its all ruls into carpet's setting manager. For example this is how carpet extra does:

// let's /carpet handle our few simple settings
CarpetServer.settingsManager.parseSettingsClass(CarpetExtraSettings.class);

It's also the same in fabric-carpet-extension-example-mod

If the extension follow the way like the 2 examples above there's no issue of syncing extensions rules to the client

commented

Yeah, but for example Skyblock (here) (and other extensions, it is documented) registers its own SettingsManager, that basically never sees the client due to this bug.

Docs in the CarpetExtension interface:

/**
* Provide your own custom settings manager managed in the same way as base /carpet
* command, but separated to its own command as defined in SettingsManager.
*/
default SettingsManager customSettingsManager() {return null;}

It's basically a way to get a custom command managed just like /carpet, and according to the docs, should be managed just like Carpet's.

Also in the example mod, see the next 2 lines from your link to find another example of the same.

commented

Ah ye, that's a thing

commented

Yeah, its for other extensions using their own command trees. Merged btw

commented

@gnembon This issue wasn't fixed by my PR, that PR included allowing client-side rules (like fogOff that can work without carpet in server) to work for other SettingsManager instances, but it didn't resolve this issue, since it didn't touch the protocol at all, and the client side command parser is explicitly disabled when server is Carpet (not the issue though).

This is still an issue.

commented

Ok found a way of fixing this without breaking compatibility, will send a PR SoonTM.

Probably after external issues are resolved.

Sent a PR.