Conversation
Farenheith
force-pushed
the
compatibility-layer
branch
8 times, most recently
from
9eefed4 to
0eb70f1
Compare
Farenheith
force-pushed
the
compatibility-layer
branch
from
0eb70f1 to
f026ee9
Compare
Farenheith
force-pushed
the
compatibility-layer
branch
2 times, most recently
from
91f2b8e to
2caf893
Compare
Farenheith
force-pushed
the
compatibility-layer
branch
from
2caf893 to
d98673b
Compare
|
There are many changes, so I'll take a look at them one by one |
|
|
| WeakHashMap<K,V> hashMap = cache.get(key.getClass()); | ||
| if (hashMap == null) { | ||
| hashMap = new WeakHashMap<>(); | ||
| cache.put(key.getClass(), hashMap); |
There was a problem hiding this comment.
Why are these maps separated depending on its class of values? I think it is not necessary
There was a problem hiding this comment.
If you have instances of ServerPlayer and ClientPlayer for the same player, equals returns true for them, which breaks the WeakMap functionality for this use-case, so I needed to separate the maps by classes, otherwise you'd get a ServerPlayer in a place where you expected a ClientPlayer
|
|
||
| public class AbstractClientPlayerWrapper extends PlayerWrapper { | ||
| private AbstractClientPlayerEntity player; | ||
| private static final WeakCache<AbstractClientPlayerEntity, AbstractClientPlayerWrapper> cache = new WeakCache<>(); |
There was a problem hiding this comment.
Though not limited to this class, WeakCache class of this static field contains java.util.WeakHashMap having AbstractClientPlayerEntity as key and AbstractClientPlayerWrapper itself as value right?
But AbstractClientPlayerWrapper has AbstractClientPlayerEntity instance as strong reference. If I remember correctly WeakHashMap's keys are weak reference but the values are strong reference. So in this case the keys which are weak reference are strongly referenced by their own values. The values are strongly referenced by WeakHashMap itself. As a result I think GC cannot collect the key objects?
There was a problem hiding this comment.
I think you're correct. To fix that I need to maintain a weak reference for the entity itself inside the wrapper, thank you for pointing that out
|
Although I've not checked all of this pull-request yet, I feel this compatibilty layer is quite troublesome to maintain. Actually there are advantages about porting between versions but porting doesn't take much time in the entire development. I wonder whether the cost of implementing this compatibilty layer and maintainance is lower than porting cost. |
|
@alRex-U Well, I've been working on this all week, and I feel the same. This looks like a whole new mod, actually, and I haven't faced the biggest issues yet. I think that porting to 1.21.1 or even 1.21.4 will bring some quite difficult barriers to overcome. I think you can close the issue and let it be. I'm interested in keeping playing with this to see how far it can go, though |
Farenheith
force-pushed
the
compatibility-layer
branch
from
6e7b8ea to
a27382c
Compare
2 participants
Regarding #369. It'll not be solved completely, it's just the start.
The idea here is to create "Wrappers" for Enttities and its child classes. The wrappers will have mirrored methods of those classes to be used across the mod. They're not always mirrored, some of them are written for better convenience.
So, instead of dealing directly with the Minecraft entity, the idea is to deal with the wrapper. WeakHashMaps are used to control a reference between the original instance and its wrapper, making it possible to obtain them easily.
Instanceof is prohibited for wrappers, and you need to use the methods "is" or "getOrDefault", in the points where you have to obtain a wrapper from an original instance.
The expected result of this strategy is to concentrate the compatibilization in the entrypoints where entities are received, and in the wrappers, that will translate the calls for each Minecraft version..
The wrappers methods must be designed for the highest compatibility between methods, in a way that in the parcool exclusive classes no merge conflicts will happen, but I know I've not there yet. I'll need to try to port it to other versions to improve it.
Finally, there are a lot of classes that would need wrappers to improve compatibility, but the plan here is to go little by little, as this tend to be a huge work and the chance of messing everything out if trying to do all at once is terrifying.
If you can analyse this PR to criticize its idea and viability, I'll appreciate it very much. The final goal is to have all the "internal parcool classes", ie, the ones that are not entrypoints for events, will not have any Minecraft, forge, neoforge, or fabric references. Everything will be moved to wrappers, where the conflict party will happen, but as each method will be just a tiny piece of code, they tend to have fewer conflicts over time.
Edit: Added Wrappers for Vector3d, which ends up generating lots of conflicts
Edit2: Adding LevelWrapper
Edit3: I've changed my goal. I'm trying to get a smooth port to 1.19.2 and I'll work on this draft to achieve this.