-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: split chunkMesh to ChunkMeshImpl #4893
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
engine/src/main/java/org/terasology/engine/rendering/primitives/ChunkMesh.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/terasology/engine/rendering/primitives/ChunkMeshImpl.java
Outdated
Show resolved
Hide resolved
I don't think we need the headlesssChunkMesh will just creating a mock object from the interface. |
I prefer to implement test object over interface than use mockito:
|
…re/split-ChunkMesh-into-implementation
engine/src/main/java/org/terasology/engine/rendering/primitives/ChunkMesh.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/terasology/engine/rendering/primitives/ChunkMeshImpl.java
Outdated
Show resolved
Hide resolved
…re/split-ChunkMesh-into-implementation
…m:MovingBlocks/Terasology into chore/split-ChunkMesh-into-implementation
@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? |
its for the chunkMeshComponent where that hook is used.. the disposal hook is just for this edge case introduced by this chunkMeshComponent stuff |
Used where? Terasology/engine/src/main/java/org/terasology/engine/rendering/logic/ChunkMeshComponent.java Lines 15 to 27 in 09b552a
|
engine/src/main/java/org/terasology/engine/rendering/logic/ChunkMeshRenderer.java
Outdated
Show resolved
Hide resolved
ah, the pointer to ChunkMeshRenderer system helped, thank you. This system is already defining an 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? |
…re/split-ChunkMesh-into-implementation
better to just keep it simple. I just moved it to use the normal events to drive disposing chunks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
engine/src/main/java/org/terasology/engine/rendering/primitives/ChunkMesh.java
Outdated
Show resolved
Hide resolved
engine/src/main/java/org/terasology/engine/rendering/primitives/ChunkMeshImpl.java
Outdated
Show resolved
Hide resolved
…esh-into-implementation # Conflicts: # engine/src/main/java/org/terasology/engine/rendering/logic/ChunkMeshRenderer.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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