Conversation
| if (mat == null) { | ||
| return null; | ||
| } | ||
| return materialMap.computeIfAbsent(mat, material -> { |
There was a problem hiding this comment.
IMO we should just remove the materialMap entirely, and cache this in the BlockState using a LazyReference like BlockType already does.
Plus, the one in BlockType can be removed and replaced with a call to getDefaultState().getMaterial().
There was a problem hiding this comment.
I'm not so sure about removing materialMap.
A LazyReference in BlockState would only survive as long as the BlockState does, and I'm not sure BlockStates are cached for as long as that materialMap would be.
From what I can tell, BlockStates can be created by a LazyReference in BlockType and BlockType's constructors are public, so anyone could create a new instance at any time, and, along with it, a BlockState.
Basically, every new BlockType(0).getDefaultState().getMaterial() would create a new BlockMaterial instance representing air.
With the current implementation, the materialMap deduplicates that and we get the same BlockMaterial instance each time.
On the other hand, what do we gain?
- Do we reduce complexity?
- Not really. We replace one field by another. I'd even argue that
LazyReferenceis a more obscure feature thanMap.computeIfAbsent.
- Not really. We replace one field by another. I'd even argue that
- Do we save memory by discarding unneeded
BlockMaterialinstances?- Most of the time, no:
- If we use a
LazyReference,BlockMaterials will be referenced by theirBlockState, so they are tied to thatBlockState's lifetime. - All
BlockStates are referenced by theirBlockType's internalBlockTypeStateList, so they are tied to thatBlockType's lifetime. BlockTypes, in turn, are mostly created on server start and held indefinitely, so nothing is ever discarded, just like with thematerialMap.- And for user-created
BlockTypeinstances, we loseBlockMaterialdeduplication, as mentioned before.
- If we use a
- Most of the time, no:
As best as I can tell, it's down to taste, so changing it is beyond the scope of this PR.
There was a problem hiding this comment.
We don't expect people to be making new BlockTypes at all, so this is acceptable. We save memory because a HashMap uses extra memory than just that required for the items in it, due to the load factor. It's also a lot faster to read two fields (the reference and its stored value) from an object we're already in, rather than do a hash lookup. Also, as soon as we hit a Java version with a stable Lazy Constants, we're likely going to replace all LazyReferences with that, so it won't be so obscure anymore.
I think we can do it outside of this PR though.
| * @return the material, or null if the material information is not known | ||
| */ | ||
| @Nullable | ||
| default BlockMaterial getMaterial(BlockState blockState) { |
There was a problem hiding this comment.
It's a bit complicated, but I think this would be best as @NonAbstractForCompatibility, and getMaterial(BlockType) should be deprecated for removal.
There was a problem hiding this comment.
Added @NonAbstractForCompatibility and the apiNote like I saw in other places.
But I don't quite understand why getMaterial(BlockType) should be deprecated for removal.
As far as I can tell, this gets the actual nms-level default block state, whereas getMaterial(blockType.getDefaultState()) merely gets the first in the list of possible states.
I don't know if that's a distinction without a difference, but I like to understand changes before I put my name on them :)
* create parity between //snow and //thaw * use layer visitor + cylinder region * fix wildcard import * make simulate snow and thaw match javadocs
TomyLobo
force-pushed
the
fullcube-double-slabs
branch
from
746fccf to
a9b08cf
Compare
TomyLobo
force-pushed
the
fullcube-double-slabs
branch
from
a9b08cf to
96ac382
Compare
4 participants
No description provided.