feat: multiblock system#68
Conversation
skaldarnar
left a comment
skaldarnar
left a comment
There was a problem hiding this comment.
If I see this correctly, this draft tries to integrate with Multi Block in a way that requires near to none changes in that module. However, this also means that it utilizes the recipe-based approach intended for multi-block formation from user interaction:
Recipe → Definition → automatic detection ⇒ multi block entity
Even though the current implementation of Multi Block does not offer this feature yet, I think it would be beneficial if something like Structure Templates should shortcut this pipeline by skipping the recipes altogether.
We could add a new event which clearly states it's purpose, namely, that its main use is for Structure Templates or similar use cases where a complete multi-block structure is placed in a (seemingly) atomic operation.
CreateMultiBlockEvent(definition) ⇒ multi block entity
The event handler for this event could be as simple as just forwarding the definition to the existing createMultiBlock method in MultiBlockServerSystem:
@ReceiveEvent
public void onCreateMultiBlock(CreateMultiBlockEvent event, EntityRef entity) {
createMultiBlock(event.getDefinition());
}If you can create the definition via a custom recipe implementation, I'm pretty sure we can also create it directly without the detour from the info we have about the template.
I'd hope that it would all still work out even without ever registering a recipe for our "StructureTemplates:Structure" multi-block type.
| import java.util.ArrayList; | ||
| import java.util.Collection; | ||
|
|
||
| public class StructureBlockDefinition implements MultiBlockDefinition { |
There was a problem hiding this comment.
This is exactly the same as https://github.com/Terasology/MultiBlock/blob/develop/src/main/java/org/terasology/multiBlock2/DefaultMultiBlockDefinition.java, therefore let's use the existing class instead of copying it.
| logger.info(pos.toString()); | ||
| } | ||
| } | ||
| return new StructureBlockDefinition(blocks, location, "Structure"); |
There was a problem hiding this comment.
I'd prefer if we'd scope the multi block type a bit more, as Structure is a pretty generic name. Just prefixing it with the module name should be enough, I think.
| return new StructureBlockDefinition(blocks, location, "Structure"); | |
| return new StructureBlockDefinition(blocks, location, "StructureTemplates:Structure"); |
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class RecipeImpl extends StructureTemplateRecipe<StructureBlockDefinition> { |
There was a problem hiding this comment.
Is there any reason why you split RecipleImpl from StructureTemplateRecipe here? Do we expect more recipe implementations within Structure Templates?
skaldarnar
left a comment
skaldarnar
left a comment
There was a problem hiding this comment.
I've got a couple of question to understand better what is happening here...
|
|
||
| import java.util.List; | ||
|
|
||
| public class SendRegionEvent implements Event { |
There was a problem hiding this comment.
This event needs documentation.
Where is the region sent to? Who should react on this event? What kind of event is this?
| Vector3ic location = new Vector3i(); | ||
| List<SpawnBlockRegionsComponent.RegionToFill> blockRegionList; |
There was a problem hiding this comment.
I think these top-level members should have some Javadoc explaining what they are used for. I would not expect that the ST server system needs a location or list of block regions on its own.
| // once the structure has been placed create the multiblock entity | ||
| EntityRef block = blockRegistry.getBlockEntityAt(location); | ||
| block.send(new SendRegionEvent(blockRegionList)); |
There was a problem hiding this comment.
Does this always create a multi-block entity for all structure templates now?
There was a problem hiding this comment.
I think this is restricted to StructureTemplates being spawned using a layered animation. So in case we want this to happen for other kind of animations or no animation we would have to send those regions separately.
There was a problem hiding this comment.
I think it is an inconsistency that this happens only for a specific animation, especially since animation and multi-block have essentially nothing to do with each other.
To be honest, I'd like to derive whether to create a multi-block structure or not from the structure template itself, i.e., only send this event if the template states that it shall create a multi-block structure.
Such information could even be paired with the type that should be used for the spawned structure.
There was a problem hiding this comment.
Looks like the other animations don't work perfectly and the game crashes and doesn't work as expected when trying to use the other animations (FallingBlocks and NoAnimation) , only the default layered animation works perfectly for me . Most of the Structures use the default Layered Animation so multiBlock should work for all of them. Not too sure where the problem lie's yet though.
| BuildStepwiseStructureComponent buildStepwiseStructureComponent = new BuildStepwiseStructureComponent(blocksPerStep); | ||
| BuildStructureCounterComponent growStructureCounter = new BuildStructureCounterComponent(); | ||
|
|
||
| location = blockRegionList.get(0).region.getMin(new Vector3i()); |
There was a problem hiding this comment.
So we just take the minimum corner of the first block region in the template as the location at which we place the multi-block root?
I'm wondering whether using the origin of the ST would be better (when you create STs in-game you start the selection from somewhere, and that block will be the origin of the local coordinate system iirc).
There was a problem hiding this comment.
I can't think of any added benefit of having the origin as the main location as the main location of the multiBlock is useful only to locate the multiBlockEntity with the necessary information. I could give this a try though.
There was a problem hiding this comment.
Alright then, let's leave it as the first minimum corner we can find. No need to change this.
skaldarnar
added
the
Type: Improvement
3 participants
Requires Terasology/MultiBlock#28
We need a way to get the region (which we get from StructureTemplates) given the location of the mainBlock. This could be done by adding the component to the blockEntity at the location or sending events and updating a Map of each vector to the corresponding regions etc. But the recipe implementations are not meant to be used as systems (didn't work out for me).