search

TheCSMods / mc-better-stats

A Minecraft (Fabric) mod that improves the statistics screen and makes it more useful.
https://modrinth.com/mod/n6PXGAoM
GNU Lesser General Public License v3.0
49 stars 19 forks source link

[v3.7.1+Quilt-1.20.X] Clients forced to install the mod when it is installed on a server #102

Closed DarkFireNinja closed 7 months ago

DarkFireNinja commented 10 months ago

What happened?

When i try to use this mod on a LAN server with my brother who doesn't have the mod installed (he is using Vanilla Minecraft), he gets this error when he tries to join my world:

**Connection Lost

Unsupported (vanilla?) client! This server requires modded client to join!**

I'm using Quilt Loader on version 1.20.1 of Minecraft.

Steps to reproduce

  1. Go to...
  2. Then do...
  3. See the bug or issue or a crash

Relevant log output or crash report

Connection Lost

Unsupported (vanilla?) client!
This server requires modded client to join!

Other installed mods

  1. First mod's name
  2. Second mod's name
  3. And so on... If you use a mod pack instead, state that and name the mod pack.

Code of Conduct

TheCSDev commented 10 months ago

Hello.

I have recently noticed that either the game itself, or the fabric loader itself, forces "clients" aka other players to install the mod as well, when it is installed on the "server". In other words, when you host a "server", whether it be a "dedicated" one or a "lan" one, for some reason the said "server" also forces all players attempting to connect to it to install this mod as well.

When developing this mod, this was never my intention, as I always wanted this mod to be "optional" on both sides, so I guess I have to figure out how I can prevent the enforcement of this arbitrary rule I do not want either.

Edit: typo and formatting fixes

DarkFireNinja commented 10 months ago

@TheCSDev Thank you for replying, I would love this mod to be fully Client-Side as well, let me know if you ever manage to fix this. Thanks!

TheCSDev commented 10 months ago

Will do. Also changed the title so people don't get mislead into thinking LAN doesn't work at all.

TheCSDev commented 10 months ago

I ran a little investigation of my own, and, I have discovered that in pure vanilla, the client has no issues connecting to a modded server that only has server-sided mods.

However! I believe I have found two mischievous culprits, its names being Architectury API and potentially QuiltMC. Apparently, the devs of the architectury mod felt that it was necessary to uh... image

So I guess that is a "soft conflict". "Soft" meaning that it does not prevent my mod from functioning, just that it somewhat interferes with my mod in a way I do not like.

Now, I am yet to discover what is causing the

Connection Lost

Unsupported (vanilla?) client!
This server requires modded client to join!

error. However, my suspicion here is that it is either QuiltMC or Architectury API. I may do further investigating to figure that part out.

Once I'm done investigating, I plan on making attempts to patch those issues out. Now, one worry I do have, is that both culprits are community-maintained (as in not by Mojang), meaning they could push out updates at any times that break my pathes, effectively causing actual conflicts (which would be a big nono).

I'm also gonna reference the following issue here: https://github.com/Superkat32/Explosive-Enhancement/issues/22 just so this reply gets automatically mentioned there, so I guess everyone on that issue can have the info I found here I guess.

Edit:

After checking the logs, I found that the following lines of code are causing my issue:

at net.fabricmc.fabric.impl.registry.sync.RegistrySyncManager.checkRemoteRemap(RegistrySyncManager.java:388) ~[fabric-registry-sync-v0-2.3.2+4df89eb277-3830ec6d52dbef83.jar:?]
    at net.fabricmc.fabric.impl.registry.sync.RegistrySyncManager.apply(RegistrySyncManager.java:284) ~[fabric-registry-sync-v0-2.3.2+4df89eb277-3830ec6d52dbef83.jar:?]
    at net.fabricmc.fabric.impl.registry.sync.RegistrySyncManager.lambda$receivePacket$0(RegistrySyncManager.java:118) ~[fabric-registry-sync-v0-2.3.2+4df89eb277-3830ec6d52dbef83.jar:?]

Interesting. And I thought it was Architectury, when it turns out it's actually Fabric. Altho now I am confused as to why it took place specifcally when I installed Architectury and not without it.

Edit 2:

Turns out it's actually likely Fabric API, and not Architectury API. I think I misinterpreted.

TheCSDev commented 10 months ago

Status update:

I am working on a separate project now, one that aims to resolve this issue not just for this mod, but other mods affected as well. I plan on publishing it as a separate mod, given this is an issue caused by the "Fabric API", and not just this mod, and it's affecting other mods as well.

As such, I feel that having a separate mod fix the issue for many other projects would be far more efficient than having every* modder patching the same issue within their own mods. Once I am done, I may end up including additional patches within this mod specifically, but we'll see...

Edit: typo..

Superkat32 commented 10 months ago

Hey CSDev! Thanks for pinging my issue because it seems you've made it much further than I ever would have xD I just ran the same test myself after finally getting into an opportunity where I could get enough players for a LAN play test(by myself) and I actually didn't have any issues. Nothing was mentioned in either player's log(from what I could see). If it helps, here is some info about my testing.

LAN Playtest Info **Modded Player:** Minecraft version: - 1.20.2 Mod List - Explosive Enhancement, Mod Menu, Fabric API, and YACL Fabric Loader Version - 0.14.22 Fabric API Version - 0.89.2+1.20.2 Was in the development environment. Host of LAN world. **Vanilla Player** Minecraft version - 1.20.2 Was launched from the vanilla Minecraft launcher.

I can only test with one player in the development environment because I don't have enough accounts otherwise. I don't think that would affect anything here though. Also worth noting that the LAN host(modded player) was on Fabric and not Quilt, which may affect something.

I'd love to stay updated with your progress if you decide to continue through with it. Out of curiosity more than anything, if it is an issue with Fabric API, would it be possible to PR the fix into Fabric API or no?

TheCSDev commented 10 months ago

Hello! Thanks for reaching out. (Sidenote: If you wanna test with multiple clients, just disable "online mode" on the server, and run multiple clients using Gradle development environment tasks. Edit here as well: For me Grade runs MC without authenticating MC accounts.)

When looking at your mod list, my suspicion is that neither of them affect the game's registries:
(edit: i tweaked the descriptions a bit)

I believe neither of the mods you used affect the game's registries in any way. Try using a mod that adds commands next time, or some other registry-related feature that can easily be ignored on the client.

Meanwhile, my project is done, and surprisingly, it's source code is super simple (technically just one simple Mixin that disables the Fabric's registry synchronization mecahnism). I published the project on GitHub (https://github.com/TheCSMods/mc-isiwiw), and am awaiting approval on CF and MR.

You can either disable Fabric's client-sided registry synchronization mod:

Or the server-sided one:

However only issue here is that doing that stuff would disable the mod part of Fabric API's ecosystem that is responsible for registry synchronization, which is not what some players would want I imagine (another reason why I turned it into a mod instead).

Edit:

Also, I do not know if what I did there with those Mixins was dangerous or not, so I hope it don't break any mods out there.

Superkat32 commented 10 months ago

I believe neither of the mods you used affect the game's registries in any way. Try using a mod that adds commands next time, or some other registry-related feature that can easily be ignored on the client.

I'm not sure if this counts or not, but my mod does have some registration for the particles.

https://github.com/Superkat32/Explosive-Enhancement/blob/b79167189e0b476bcd4e2bf350b8c2e331ea4189/src/main/java/net/superkat/explosiveenhancement/ExplosiveEnhancement.java#L28

There's also client-side registration in the client initializer, but I'm not sure if that is important here or not.

I had asked for some help on the issue on my repo a few days ago regarding this and everybody generally responded try messing around with the registries in that main initializer there. I was going to do that last night until I found out everything seemed to work without needing any changes, so I'm not sure.

Edit:

I should probably also mention that I've only recently started working with packets and networking for Minecraft. I'm not super aware of the limitations of the dedicated server and the integrated server, which I feel is important here.

TheCSDev commented 10 months ago

Ah I see. I didn't even notice your mod has to register particle types (ngl I didn't even check). Interestingly, Fabric API's registry synchronization mechanism does mention particles too in its initializer:

RegistryAttributeHolder.get(Registries.PARTICLE_TYPE)
                .addAttribute(RegistryAttribute.SYNCED);
//comment by me: ^ isn't that thing above supposed to trigger forced synchronization?

So in that case, I'm not really sure why your mod is unaffected either. Perhaps Fabric API doesn't really care as much about particle types? I don't know. But I guess one thing we likely do know, is that Quilt likely either does care, or just makes clients install all mods on a server.

TheCSDev commented 10 months ago

I got the chance to do some testing again, and it appears QuiltMC is being more stubborn here, having a more sophisticated mechanism for enforcing mod download rules. Attempting to use ISIWIW to disable the registry synchronization mechanism only results in the Quilt server thinking the client is attempting to join on vanilla, and the server then kicks the client. Attempting to apply the mod on the server itself has no effect (likely because Quilt's version of the mechanism is in another Java package or smth, idk rn).

I suppose I will have to look into if it's possible to get the solution working for Quilt servers. Until then, this issue technically still isn't resolved for Quilt.

DarkFireNinja commented 9 months ago

@TheCSDev Wow it seems like a missed a lot. Thank you so much for working really hard on fixing this issue! Would it be possible to create a mod just like ISIWIW but for Quilt? Thanks!

TheCSDev commented 9 months ago

@TheCSDev Wow it seems like a missed a lot. Thank you so much for working really hard on fixing this issue! Would it be possible to create a mod just like ISIWIW but for Quilt? Thanks!

Mhm. I am actually planning on adding full Quilt support to it as of right now. So the initial plan always was to have it support both Fabric and Quilt. Given Quilt is a bit more "stubborn", some extra work is needed to get ISIWIW to fully support Quilt. But overall, yes, I plan on having it support both.

DarkFireNinja commented 9 months ago

@TheCSDev Wow that sounds amazing, I'm very excited about that. And like you said, creating a mod that can fix the LAN issues instead of having to ask each mod creator to fix the issue themselves, is a very good idea. Thank you so much once again!

MaixGit commented 7 months ago

Bump. Is there any updates on this being fixed?

TheCSDev commented 7 months ago

Bump. Is there any updates on this being fixed?

Hello. Technically yes and no. Basically, there are good and bad news to this question:

TLDR;
Just disabling all "optional server-sided features" should work. If it doesn't, then that's a bug that needs further looking into.
Hopefully that helps. Have a great day!

MaixGit commented 7 months ago

Thank you!

I'll try the new settings in a dozen hours when a friend gets on.

Do you think it would be possible to make a mod that can configure or block such settings and features in other mods? A sort of compatibility forcing mod?

TheCSDev commented 7 months ago

As mentioned earlier in this issue's conversation, I did make ISIWIW, a mod that prevents Fabric API from enforcing registry synchronization rules (tho Quilt support is partial). On the positive side, anyone with this mod can join with any mods at any time. However a drawback is that the server won't even be able to enforce "required" stuff such as blocks, items, and entities, meaning a client could accidentally attempt to join without a required mod that adds modded blocks and stuff (which may or may not be bad, I don't 100% know).

MaixGit commented 7 months ago

Yeah I did see that, and I'm not sure if it's possible, but I was thinking more like an individual toggle for each mod (on the lan host side).

So that mods which are mostly clientside can be toggled, and won't cause desync regardless, such as Falling Leaves, Water Drip Sounds, Music Control, Visuality, or Wakes, as they shouldn't interfere with players who don't have ISIWIW or the mod.

Also, I'm currently using Fabric.

TheCSDev commented 7 months ago

Oh I see what you mean now. That's an interesting idea. Altho it isn't a feature on feature on Fabric's end, it probably is possible to mod that in somehow. To be honest, I don't really know how tho, as I didn't look into how the registry synchronization mechanism works any further than finding out how to disable it.

MaixGit commented 7 months ago

I just tried disabling the serverside features of Better Stats, but it seems the issue is actually with your API lib (TCD Commons). As when my friend tries to join, that's the mod which pops up as sending the false registry packets.

TheCSDev commented 7 months ago

Hm, that's odd, and definitely not supposed to happen.
Any chance you could provide me a screenshot, or even better, a log file, that goes into detail as to what issue took place?

MaixGit commented 7 months ago

Of course! Sorry if the log is a bit messy, I've got quite a few QOL mods

Here's a screenshots image

The host log
[removed by TheCSDev]

And player log
[removed by TheCSDev]

TheCSDev commented 7 months ago

Ah that issue. After looking into the log, I found the following on the clinet:

[22:48:41] [Render thread/ERROR]: Received unknown remote registry entries from server
[22:48:41] [Render thread/ERROR]: Registry entry (tcdcommons:player_badge_identifier) is missing from local registry (minecraft:command_argument_type)
[22:48:41] [Render thread/ERROR]: Registry entry (tcdcommons:stat) is missing from local registry (minecraft:command_argument_type)

So basically a player-badge-related and a stat-related command-argument-type are still present and being registered by this mod, despite the feature not being enabled in the config. Now I know Fabric API says it's tcdcommons's, and while technically true, those are still handled and controlled by betterstats, so I guess the issue could be with this mod ignoring the config when registering command argument types.

I will look into resolving this if possible, as I did not intend on registering anything server-side-related when commands are disabled. In the meantime, I suppose ISIWIW would have to do.

Also, in my honest opinion, while I understand why Fabric API would force clients to install certain mods that may be required, I do not like how they enforce the rule on practically everything, including stuff the client technically doesn't need (aka stuff like commands). This rule basically makes it difficult to make server-sided utility mods that add things like administrative commands, without Fabric tripping up and forcing the client to install them as well. Worst part is that Fabric provides no ways for the end user or mod developers to control this behavior either (not that I know of that is).

Either way, thanks a lot for the logs as well. Tho I will remove them so as to hide personal info from the public, because I think I saw IP addresses and stuff, so I'll just err on the safe side. Thanks again, I'll look into this further, and have a great day!

MaixGit commented 7 months ago

Yeah sound good! I manually removed usernames and any personal details apart from ip addresses, because we just restarted our routers. Thank you though.

Do you think Fabric would ever improve their registry sync system or provide a way for mod devs to work around it? I really like Fabric at the moment with the mods I'm using and switching to something like quilt would be quite a trek.

TheCSDev commented 7 months ago

Do you think Fabric would ever improve their registry sync system or provide a way for mod devs to work around it?

I'm not sure. I personally don't see that happening. I suppose one could open a suggestion issue on the Fabric's GitHub repo however.

...and switching to something like quilt would be quite a trek

True, and, Quilt is a fork of Fabric, so Quilt has the exact same mechanism present.

-

Now a "fun fact" about Quilt is, they intentionally chose to make the system a bit more "strict", hence why ISIWIW has limited support for Quilt.

Basically, on Fabric, Fabric server just tells the client "here's what I have installed", and the client just "kicks itself" when it notices that it doesn't have a mod installed.

On Quilt however, the server has an additional step that requires the client to confitm to the server that it has the "necessary" mods installed. So on Quilt, if a client refuses to send a message to the server that confirms the client has the mods installed, the kicking will then be handled by the Quilt server.

Additionally, I think the system may also involve the client having to tell the server what mods it has installed, which is yet another convoluted step that makes things more complex and difficult in my opinion (altho I'm not 100% sure if this step ever actually happens).

ETA (edit to add):
On Quilt, I actually attempted to make the ISIWIW mod disable the mechanism on the server-side like I did with Fabric, however unlike on Fabric, this seemed to have no effect on Quilt, hence the "limited Quilt support".

TheCSDev commented 7 months ago

I have recently published an update.
Hopefully it resolves the issue of "disabling /stats" solution not working. Now disabling the command should prevent this issue from taking place.

rvbsm commented 7 months ago

Hi! I've also struggled with this issue in my mod and want to help you solve it.

The problem is not with Quilt, but with the registry update. If you are registering an argument type, you need it on both sides, because autocompletion on the client will be handled by the client. Perhaps the issue should be moved to TheCSMods/mc-tcdcommons.

I have solved it by hardcoding the suggestions with use of IdentifierArgumentType (the code was copied from 1.20.4/StatArgumentType.java):

argument("stat", IdentifierArgumentType.identifier()).suggests((context, builder) -> {
    StatType<?> statType = null;
    try {
       statType = RegistryEntryArgumentType.getRegistryEntry(context, "stat_type", RegistryKeys.STAT_TYPE).value();
    } catch (CommandSyntaxException | IllegalStateException | ClassCastException e) {
    }
    if (statType == null) return IdentifierArgumentType.identifier().listSuggestions(context, builder);

    @Nullable Iterable<Identifier> suggestions = statType.getRegistry().getKeys()
          .stream()
          .map(RegistryKey::getValue)
          .toList();

    return CommandSource.suggestMatching(
          StreamSupport.stream(suggestions.spliterator(), false)
                .map(Objects::toString),
          builder);
});

https://github.com/TheCSMods/mc-better-stats/assets/39232658/dab63d59-d2ca-443e-9dc1-24e7ab6385fd

TheCSDev commented 7 months ago

Beautiful. Thank you so much!
I didn't even notice or know .suggests() is a thing, and the entire time I thought I gotta register my own ArgumentTypes for this kinda stuff. I just made the changes you mentioned to my 1.20.1 code real quick, and they work flawlessly. Haven't yet tested if they resolve this issue or not, will do that once I handle other game versions and TCDCommons's command for player badges that also has its own ArgumentType.
Also, if I understood correctly, the reason the client has to have the ArgumentTypes registered is because autocompletion is handled on the client-side? That's good to know, and makes sense in a way. If that's the case, then that explains this issue's presence.
Again, thanks a lot! Will work on implementing your solution now.

rvbsm commented 7 months ago

Glad to help :relaxed:

[...] the reason the client has to have the ArgumentTypes registered is because autocompletion is handled on the client-side?

AFAIK, yes.

Also, off-topic, translations are also handled by the client, so commands output for players without mod will be, for instance, commands.statistics.edit.output. There is Server Translation, so you can either hard-code messages in English or use this library.

TheCSDev commented 7 months ago

Turns out the resolution works flawlessly.
So I suppose it never was an issue with Fabric API, and it was me the entire time. Glad that's resolved now. As for the translations, yea, I think I will handle that later as a separate issue. Thanks a lot again!