Apparently it's encouraged to make your AttachmentTypes immutable but that seems a bit dumb for large objects with lots of fields inside fields etc. If i want to change just one value, why recreate the entire object?

I suggest making the object.equals check optional. Just add a boolean to attachmentType, requiresImmutability, or whatever

I found a few workarounds around this issue, which all seem really bad

1. whenever i want to sync things, first set my object to an empty instance, before setting it to my real data, this avoids the stupid object equals check, this is bad because it syncs the data twice now
2. use the codec to load my object back and set that instead, this is a lot of overhead for a simple field change but it avoids the double sync
3. sync the data myself when i need to.
4. wrap all my attachments into another object/record and increment a simple transient counter field, this should avoid the equals check

Or is there a good reason to force immutability on attachments?

They really should be immutable, one of the big issues it solves is having the same attachment data instance on multiple targets. You can either do it on purpose or it saves you from issues where it happens accidentally. Being immutable also forces you to correctly use the API to update the values, without doing that the target may not know that the attachment needs re-syncing or saving. If we could have done we would have forced them to be immutable types when using the API, but java doesnt allow that.

If the amount of data really is that big maybe storing it as attachments isnt the way to go. You could maybe store an ID/UUID in the data attachment and use that to query the data from an immutable place. Maybe store it as world data using PersistentState. But you will be on your own with syncing if thats something you need.

Another option is having multiple data attachments, does it all need to be 1 big object?

I have like 10 attachmenttypes and they're all sorta big objects. It's impossible to cut it up any more. For example all players can unlock spirits and each spirit has their own equipment slots, levels etc to store.

There's many times i want to change a value, without syncing it. The player doesn't need to get the entire 100 itemstacks synced to him just because i updated a single spirit's experience value (20 times a second sometimes)

Ok maybe, i split things, so items and levels are in different attachments. I still need to send sync updates every time a player kills a mob, but why, if the exp is only shown inside a screen that you have to access?

Another simple attachment is my CooldownData class, which is just a map<String,Integer>. Cooldowns go down every tick, i don't need to sync them because the client knows how to simulate ticks too. I just sync the data to tell the client to start a cooldown, and it ticks it down on its own, without needing 20 sync packets a second

To me it seems bad to use immutable types for this, and this attachmenttype system looks like the best fit for storing data on entities, so the fact that it tries to force immutability is really weird. If attachments are meant to be immutable, can we have a different system to attach mutable data?

I really like how simple and nice to use the system is, i just need to store data, sync it when i want to and make it persist + persist on player revive etc.

one of the big issues it solves is having the same attachment data instance on multiple targets.

Also can you explain this?

I have like 10 attachmenttypes and they're all sorta big objects. It's impossible to cut it up any more. For example all players can unlock spirits and each spirit has their own equipment slots, levels etc to store.

There is nothing stopping you from having 10 attachment types for each spirt. I dont know exactally what your mod needs but this kind of pattern comes to mind:

public class Spirt {
	public static Spirt EXAMPLE = new Spirt(Identifier.of("mymod", "sprit_1"));

	private final AttachmentType<Integer> level;
	private final AttachmentType<Integer> cooldown;

	public Spirt(Identifier identifier) {
		level = AttachmentRegistry.create(
				id(identifier, "level"),
				builder -> {
					builder.syncWith(PacketCodecs.INTEGER, AttachmentSyncPredicate.all());
				}
		);
		cooldown = AttachmentRegistry.create(
				id(identifier, "cooldown"),
				builder -> {
					builder.syncWith(PacketCodecs.INTEGER, AttachmentSyncPredicate.all());
				}
		);
	}

	public Facade get(AttachmentTarget target) {
		return new Facade(target);
	}

	public class Facade {
		private final AttachmentTarget target;

		private Facade(AttachmentTarget target) {
			this.target = target;
		}

		public int getLevel() {
			return target.getAttachedOrElse(level, 0);
		}

		public void setLevel(int value) {
			target.setAttached(level, value);
		}

		public int getCooldown() {
			return target.getAttachedOrElse(cooldown, 0);
		}

		public void setCooldown(int value) {
			target.setAttached(cooldown, value);
		}
	}

	private static Identifier id(Identifier identifier, String suffix) {
		return Identifier.of(identifier.getNamespace(), "%s_%s".formatted(identifier.getPath(), suffix));
	}
}

And then you can easily use it like:

PlayerEntity playerEntity = getPlayer();
Spirt.EXAMPLE.get(playerEntity).getCooldown();

There's many times i want to change a value, without syncing it.

The above example gives you full control over what is synced you can choose exactaly what values are synced or not, the syncs are also batched so updating multiple values will only send 1 packet.

I still need to send sync updates every time a player kills a mob, but why, if the exp is only shown inside a screen that you have to access?

I wouldnt use data attachments to sync this to a screen, vanilla's ScreenHandler and fabric's ExtendedScreenHandlerType classes are designed exactally for syncing data only when a screen is opened.

I just sync the data to tell the client to start a cooldown

This is good, but it sounds like you might need some custom packets to handle this tbh, I guess another option would be to store the world time that the cooldown completes and not the number of ticks remaining. I could be tottaly wrong but I believe the world time is synced with the client.

Also can you explain this?

Right now you can have 2 entities that both have the same object attached to them, if the object is mutable updates to one of the objects would affect both entities, but without triggering the syncing, events or saving for the other leaving you in a weird state.

From what you have explained I think that data attachments would be the best way to go, I think generally they better geared to contain only small amounts of data each, but there is nothing stopping you from having many attachments. I dont think having big mutable objects is going to solve your problems, you are still going to struggle to sync them when needed, you need to make sure that you mark the target as dirty so it gets saved when you change a value and you are going to run into possible issues if the object gets shared between multiple. All of these issues are solved with immutable data. As I have shown above you can always create a Facde that allows you to deal with the data as if it was one object but the fields are replaced with individual attachments.

Interesting to split each part into a single attachment. Though is there a problem if my spirits can be added with datapacks?
At least in forge, registries are frozen way before we load datapacks. I guess i could register 100 dummy attachmenttypes and the datapacks would tell which number attachment to use, though that's a bit weird

I also never save a single instance of my data to more than 1 target. They're all unique.

Humm, the datapack thing does complicate this a little, the client does send the attachments it can handle during configuration, meaning you could reconfigure after adding attachments but there is no way to unregister them. It wasnt really designed with this in mind. An alternative option would be to store each value as a map with the type as the key, its not perfect but might be a good middle ground?

I think im going to close this PR, you have a few options either keep with your single mutable object, and correctly implement .equals on it and be careful with how you use it. Split the attachments up or do something more custom either using attachments or not.

Maybe there is scope to better support attachments that are only present with datapacks but its not immediately clear to me how that would work.

Sure, i'll just use workarounds, it just seemed a shame not to simply offer an option to do mutable attachments without needing to use workarounds..

You can use mutable attachments though, as long as you correctly implement .equals. Its not reccomended but no one is going to stop you.

Hm ok, i thought the Object.equals doing == make it impossible for me to force a sync because my modified object is always the same instance..

You can use mutable attachments though, as long as you correctly implement .equals. Its not reccomended but no one is going to stop you.

You actually can't because Objects.equals checks for referential equality first (a == b) and only if that fails does it call the equals() method. So mutable objects will just never sync any changes to the client.

Though I do agree keeping attachments immutable or using the Facade pattern described above is a better way to go about it.

Yeah that's what i assumed but wasn't sure.
I can still find 10 different workarounds but this feels really bad for a lot of us who are used to mutable objects. It seems to just create issues for us, like this silent bug where an equals check stops sync from working while the class doc only says it's encouraged, not that you will actively be punished

Ah, my bad I didnt realise that Objects.equals checked the referances, I wouldnt be against changing this to only use the equals() method. I agree that we shouldnt have slient issues like this, even if not encoraged.

I don't think Objects.equals is the problem here. x.equals(x) is required to always return true by spec anyway, and would still return true even for correctly implemented mutable objects. Instead, for mutable attachments to be supported, there would need to be some kind of isDirty notion for them.

My first idea was to just have a simple boolean in attachmenttype, isMutable or isImmutable, but attachmenttype is a record for some reason so i don't think you can add an optional field or method like that without breaking all mods

I guess the builder can just add to a hashmap<attachmenttype,Boolean> or any extra optional data needed

Then just try to enforce immutability on immutable types only. I don't think any setdirty is needed, things already update if you call .set , as long as the equals check is removed

Edit my bad, AttachmentTypeImpl is a record, not AttachmentType

I would probably just add the field and overload the ctor

Knowing that the object is mutable can fix correctness by syncing even if it's the same object, but it would introduce performance issues by syncing even when it doesn't need to. A dirtiness notion is needed to avoid this.
Edit: if you accept that it will sync every time you set it then maybe the field is enough

More granular options could work.. Attachmenttypes could have a SyncConfig, with stuff like: shouldSyncOnSet, syncCooldown and maybe something else i can't think of