Estecka / mc-invariable-paintings

Fabric mod that turns paintings into collectibles
https://modrinth.com/mod/invariable-paintings
GNU Affero General Public License v3.0
4 stars 1 forks source link

Fix #1 (Client-server desync when player attempts to place painting in invalid space) #2

Closed Andrew6rant closed 1 year ago

Andrew6rant commented 1 year ago

My code right now basically just redirects PaintingEntity.placePainting into a similar function that also takes into account the player's held item context.

Ideally, I would rather inject into PaintingEntity.placePainting and change

list.removeIf((variant) -> {
    paintingEntity.setVariant(variant);
    return !paintingEntity.canStayAttached();
});

into

list.removeIf((variant) -> {
    paintingEntity.setVariant(variant);
    // registryEntry is the itemStack's NBT data turned into RegistryEntry<PaintingVariant>
    return registryEntry != null && variant == registryEntry && !paintingEntity.canStayAttached();
});

but I couldn't manage to nicely store the player's item context in PaintingEntity. My solution should still be compatible with all mods unless they also modify painting place logic.

Estecka commented 1 year ago

I took a spin at your suggestion, and figured that instead of trying to inject the check inside placePainting, we could do it directly from the DecorationItemMixin, by post-processing the function's return value.

I've pushed those changes to a separate branch for now. If you don't find any more issues with it, I'll pull this version instead.

Andrew6rant commented 1 year ago

Oh yeah, looking at the branch - it is much nicer code. I didn't need the accesswidener, haha.

Estecka commented 1 year ago

Now that I get a closer look at placePainting, the bulk of what it does is try to find a suitable variant. I could skip the function entirely for the sake of optimization