refactor: split chunkMesh to ChunkMeshImpl

[pollend]

This introduces an interface for ChunkMesh. should let us mock in the interface when we want to test some rendering code and other graphics API's down the line. still not sure about some of these methods that I'm keeping in the interface. I want to move some of this metric information somewhere else but that is something to consider later.

PR: #4786

[keturn]

A few quick notes so far. I hope to give it another look later.

Coming from Python, the practice of making an Interface/Class separation purely for the purpose of internal tests always feels weird. We'd just 🦆-type it in Python, right? So I end up second-guessing this sort of thing, even though I often do prefer it to Mockito.

I notice you did add a HeadlessChunkMesh implementation of the interface. Is that something a headless server will use? Or does the HeadlessGraphics subsystem already skip over all the engine.rendering.* stuff?

[pollend]

A few quick notes so far. I hope to give it another look later.

Coming from Python, the practice of making an Interface/Class separation purely for the purpose of internal tests always feels weird. We'd just duck-type it in Python, right? So I end up second-guessing this sort of thing, even though I often do prefer it to Mockito.

I notice you did add a HeadlessChunkMesh implementation of the interface. Is that something a headless server will use? Or does the HeadlessGraphics subsystem already skip over all the engine.rendering.* stuff?

I don't think we need the headlesssChunkMesh will just creating a mock object from the interface.

[DarkWeird]

I prefer to implement test object over interface than use mockito:

1. Mockito add duration around 200-500ms to every test with using mockito.
2. I think that if you should use mockito - you have architecture problem.
3. Copy pasted mockito code from test to test(also feels noisy)

javadoc did get fixed

[pollend]

@keturn we probably want to keep the chunk mesh hook. it will be used for this: Terasology/FallingBlocks#9

[keturn]

we probably want to keep the chunk mesh hook. it will be used for this: Terasology/FallingBlocks#9

How is that related to ChunkMesh.disposalHook? The FallingBlocks PR was a bit too big to look at in github web, but I checked it out and searched for "disposal" (or "dispose") in that branch and came up empty.

If we do have a use case for disposalHook, that's fine, but my question from earlier still stands: Can we get rid of the code duplication between ChunkMeshImpl.dispose and ChunkMeshImpl.disposalHook().dispose(), or are my sleepy eyes reading it wrong and it's not actually a duplication?

[pollend]

we probably want to keep the chunk mesh hook. it will be used for this: Terasology/FallingBlocks#9

How is that related to ChunkMesh.disposalHook? The FallingBlocks PR was a bit too big to look at in github web, but I checked it out and searched for "disposal" (or "dispose") in that branch and came up empty.

If we do have a use case for disposalHook, that's fine, but my question from earlier still stands: Can we get rid of the code duplication between ChunkMeshImpl.dispose and ChunkMeshImpl.disposalHook().dispose(), or are my sleepy eyes reading it wrong and it's not actually a duplication?

its for the chunkMeshComponent where that hook is used.. the disposal hook is just for this edge case introduced by this chunkMeshComponent stuff

[keturn]

Used where?

Terasology/engine/src/main/java/org/terasology/engine/rendering/logic/ChunkMeshComponent.java

Lines 15 to 27 in 09b552a

	@Replicate
	public ChunkMesh mesh;
	@Replicate
	public AABBf aabb;
	@Replicate
	public boolean animated = true;
	public ChunkMeshComponent() { }
	public ChunkMeshComponent(ChunkMesh mesh, AABBf aabb) {
	this.mesh = mesh;
	this.aabb = aabb;
	}

[keturn]

ah, the pointer to ChunkMeshRenderer system helped, thank you.

This system is already defining an onDestroyMesh lifecycle handler for the component. Why do we need to do this reference-tracking stuff instead of doing disposal there?

If we can't use normal component lifecycle events, could we at least make this a WeakReference instead of a PhantomReference, so we can call the normal dispose method on the reference?

(Please include the answers to both of the above in ChunkMeshRenderer's javadoc.)

If we do have a use case for disposalHook, that's fine, but my question from earlier still stands: Can we get rid of the code duplication between ChunkMeshImpl.dispose and ChunkMeshImpl.disposalHook().dispose(), or are my sleepy eyes reading it wrong and it's not actually a duplication?

[pollend]

ah, the pointer to ChunkMeshRenderer system helped, thank you.

This system is already defining an onDestroyMesh lifecycle handler for the component. Why do we need to do this reference-tracking stuff instead of doing disposal there?

If we can't use normal component lifecycle events, could we at least make this a WeakReference instead of a PhantomReference, so we can call the normal dispose method on the reference?

(Please include the answers to both of the above in ChunkMeshRenderer's javadoc.)

If we do have a use case for disposalHook, that's fine, but my question from earlier still stands: Can we get rid of the code duplication between ChunkMeshImpl.dispose and ChunkMeshImpl.disposalHook().dispose(), or are my sleepy eyes reading it wrong and it's not actually a duplication?

better to just keep it simple. I just moved it to use the normal events to drive disposing chunks.

[keturn]

I just moved it to use the normal events to drive disposing chunks.

That does look a lot simpler for the Renderer as well as the dispose method!

CheckStyle has a few notes for you that you may want to address before merging this. I recommend turning on IntelliJ's “optimize imports on the fly“ setting—or, if you don't like it doing stuff on the fly, at least the “optimize imports“ option in the “before commit“ section.

[keturn]

Okay, I think that's everything! And checks pass and I can still see chunks in game. Although I did run out of RAM pretty quickly this run.

How would I know if things weren't getting disposed of as they should?