Disable dynamic block allocation. #4036
Conversation
4Denthusiast
added
Topic: Stabilization
|
I was able to validate this with two clients. its probably a lot better then trying to hotload blocks on the fly since it adds its own set of complications that i'm not sure we are ready to deal with. |
Re-enable multiple blocks per shape in a more controlled manner, requiring all of them to be registered in the block definition. Blocks without a shape now default to cube rather than freeform. Existing block files should be usable unchanged except in the case where the freeform feature was actually used, in which case the list of possible shapes will have to be added.
|
This pull request fixes 1 alert when merging 35f73eb into fe4f808 - view on LGTM.com fixed alerts:
|
These classes existed to update things when new block families were registered, therefore they're no longer necessary except the parts that run during loading, which no longer need separate classes.
|
This no longer allocates excessive numbers of blocks, so (once module updates are ready), it should be ready to merge. Blocks with variants of multiple shapes are now required to list their possible shapes in the block definition file, in the |
|
This pull request fixes 1 alert when merging 86c4d0b into fe4f808 - view on LGTM.com fixed alerts:
|
| @@ -48,17 +51,25 @@ protected void doReload(BlockFamilyDefinitionData blockFamilyDefinitionData) { | |||
| } | |||
|
|
|||
| public boolean isFreeform() { | |||
There was a problem hiding this comment.
isFreeform is a bit strange in this context. never really quite understood the reasoning and how this fit into blockfamilies. at this point it just checks if the block has shapes defined for it. would it be worth deprecating and making getShapes return Optional<ArrayList>.
There was a problem hiding this comment.
Returning an Optional in this case wouldn't be better than just optionally returning null. I could just inline this method (it isn't used in many places), but I just kept it like this because block family definitions with the shapes attribute are treated a bit differently from ones without that attribute (the family can vary according to the shape, so for blocks with multiple shapes, the family is given less control over the construction of the block), so it seemed appropriate.
There was a problem hiding this comment.
What's the difference between an empty list and null/Optional.empty()?
Change the way the BlockManager is handled in several tests, so that it is initialised after the block definition assets are set up.
|
This pull request fixes 1 alert when merging a7f9784 into fe4f808 - view on LGTM.com fixed alerts:
|
|
So I've been meaning to comment on this for a long time, but it feels kinda hard to just sort of mentally eyeball its long term effects. I agree that simplifying the code would help for right now and fix some bugs, but constraining blocks to shapes up front seems limiting long term. For an example the ChiselBlocks module is just a gigantic set of textures in an Is there a way we can add some sort of less lazy loading of block ids, where those we predefine will behave better with this PR, meaning the bug fixes apply and overall quality goes up, but we have an option to expand later, perhaps via a simple server admin / game update utility command that can hard- assign additional ids with the game/server offline, then we have a new static list of ids next startup / client connection? So in that case if there's a cool new shape in SR you want to make available to CB you just go pick it in some sort of listbox of available blocks/shapes? I'm not personally going to lose sleep over losing functionality if we can see some clear future plan to help regain some lost flexibility, ideally with fewer bugs. I however don't think it is a reasonable permanent position to forego all dynamic block creation/addition, forever. Both options seem painful - more complexity in code, or more complexity in admin. Or less flexibility overall. Two other related topics / concerns:
Some of that might be reduced to game design choices / target game style. If you can't architect something to your liking just make up an in-character reason to handwave it to a more simple setup :-) Also sending a ping to @Qwertygiy as I think he's been working on making shapes more complex/flexible How does Minecraft / Minetest / others handle this, I wonder? Have they just gotten the complexity sorted out? Are we barking up the wrong tree simply because nobody dares dig through the code as-is to fix ugly bugs? That wouldn't be a first, I expect. Regardless of case and future here I do want to thank you @4Denthusiast for the effort you've put into this. I know it isn't trivial. I just also can't help feeling rather conflicted about it 😕 I really don't want that to reflect poorly on your work - I'm just one person and don't want an out-sized influence on this. I just want to make sure we cover all our bases so the community doesn't wonder a few years down the road why we don't have lazy block ids, with contributors then either having forgotten or never known the pain from this topic. |
You would have to include a block file for anything that's not just a cube, but you wouldn't have to update every one of them to add new shapes if they're all based on the same template.
Even in the current state of the PR, if additional blocks become available between game loads (either by additional block files being added or by additional shapes being added to an existing block), they will be added just fine. It's only adding blocks part way through a run of the game that's the problem. It also sort of sounds like you're suggesting leaving dynamic block allocation as-is, but allowing pre-defined shapes lists for freeform families and eagerly loading at least those for every block so that it's used less often. This would be worse than either extreme, because it has the complexity of both, and systems that require a list of all the blocks still have to take into account dynamic block allocation.
I think that in almost all cases, using an entity for something like this would already be a better option. In the case of shrinking multiple blocks into one composite block, making those actual block types would probably already be more difficult, and would only be much more efficient if large numbers of identical composite blocks were used. The sphere watermelon case would be one example where the existing system (with FreeformFamily) is actually a good fit, but it's quite contrived to be a good fit for the current system, and how many different vegetables and vegetable moulds do you expect to have anyway? Sphere watermelons are plausible, but fence watermelons, torch watermelons and cactus watermelons less so. In order to make dynamic blocks like this, you'd also have to transmit the assets defining them to all the clients in advance of registering it with the BlockManager, and similarly you'd have to somehow store these assets and re-load them before the RegisterBlocks loading step, which looks pretty much impossible without making these dynamic blocks an engine feature, because RegisterBlocks is one of the first loading steps. More to the point, as far as I know Terasology doesn't currently have any features that use dynamic block allocation except freeform families. It seems like it's more of a solution looking for a problem than something that's actually necessary for likely future expansions of the game.
It's been a long time since I worked on Minecraft modding, but I don't think they had dynamic block allocation like us at all. |
|
The one remaining test failure doesn't look like it has anything to do with this PR, so I assume it's pre-existing. |
|
This pull request fixes 1 alert when merging 15cecd0 into fe4f808 - view on LGTM.com fixed alerts:
|
|
Yep the flakey replay tests are pre-existing, no worries there 👍 And I do think I probably am missing some of the distinctiveness between different related concepts here, partly why it is hard for me to comment particularly well.
Oh so the block id list essentially refreshes between game loads? That's good to know.
Ehh, I guess I was looking for a way to retain functionality but in a less dynamic way, somehow? Not sure. You're right in that it could easily become a worst of both. I started wondering if it made sense to change the free form family to a very limited set of shapes (full block, slab, a few others) then allow any generic / auto block to belong to that by default, but that's a slippery slope as well.
I wonder - could a hybrid approach work where you can create composite blocks at runtime, using entities, then when the game reloads it can do an optimization pass where primitive new blocks get ids assigned and their entity removed? Then over time you can grow the block list dynamically in-game yet minimize entity bloat
Right now IIRC the BlockNetwork approach actually only uses one entity for a whole network, with no entity-per-block for the wires. That may well be a good candidate for that separation of appearance from function thing.
Interesting - I guess they resolved the block id conundrum some other way? Or just increased the id size 🤔 Thanks for the comments :-) I'm not suggesting we try to address this perfectly right now, especially if nothing really uses the dynamic allocation at the moment. Just curious what we might think up as a future feature enhancement should it be needed one day. |
Allowing freeform families to take on a limited set of shapes was actually the approach I took in a previous version of this PR. You'd still end up with at least 6 blocks per freeform family (cube, slab, 4 stairs, possibly column, possibly 16 fencepost variants, etc.). Also freeform is currently (without this PR) the default, but even if you changed that, you could still end up with the majority of the block IDs taken up by ChiselBlocks quite easily. Also, speaking of ChiselBlocks, it would definitely need to be canged if this PR gets merged, but an extraData field would actually be a pretty good fit for it. You could have one chisel block of each shape, and an extraData field specifying the texture (or vice-versa). In a world with, for example, FlowingLiquids also enabled, that data might even fit into the same field as the liquid data (as none of the chisel blocks are liquid, or have any other special properties that would require other hypothetical specific extraData fields), so in that case it wouldn't even take up any extra memory per chunk.
This would still run into the problem I described earlier where loading the block definition assets before block registration would be hard to do.
Even if it's one entity per wire network, there's nothing stopping it from including this information about fancy composite blocks in its network too. If you could fit multiple wiring components (like electrical components, not entity Components) into one block space, the entity that stores the state of all the wiring would still need to store information about each of these components anyway. |
I've run into that desire several times, particularly with an old idea of tracking aquifers / soil humidity in solid blocks, water level in liquid blocks, and wind strength (or pollutants?) in open blocks. Loooong time ago, that one, and I wasn't sure how efficient it would be vs hassle. The new extra data stuff seems a lot more flexible than in the olden days. Curious what potential there might be there. Could extra data itself be used for shapes, rather than textures? Paired with an overhaul a la https://trello.com/c/w4q4jhza/46-separate-block-shape-function-from-rendering-its-visual-appearance - one id for texture, one id for shape (potentially leading into some amorphous shapes for things like wires or fences that could be determined based on neighbors). I figure that way we'd suddenly have an immense id range no longer needing to mix the two. But I dunno how that works with the actual block registration and such (even with the two ids separate if you make a new block mid-game that'd still be a "new" combination / block)
Fair enough 👍 |
Yes, an extraData field could be used for shapes. If you mean doing that just for ChiselBlocks, it would be a bit awkward in that you'd have to re-implement some of the existing behaviour of block families (e.g. placing stairs in the correct direction depending on how they're placed), which is why I suggested putting the texture in the extraData field instead, because that would be a bit simpler. In either case, changing the rendering of a block based on its extraData values is currently quite awkward. I feel like the BlockMeshGenerator ought to be changed at some point to make things like this, or changing the rendering of fences based on their surrounding blocks, more convenient. If you mean storing the shape of every block in the world in an extraData field, that could work, but it seems a bit over-the-top. It would make things like the SupportRequiredFamily more complicated, because they'd have to keep the shape in sync with the block ID, which determined the functionality. |
|
So we discussed this some during the office hours today and I'm hoping to get some more feedback written up here on the PR, pinging some maybe relevant contributors: @pollend @skaldarnar @jdrueckert @keturn @casals @DarkWeird I've written up a bunch of stuff above already, including several of the potential mitigation ideas to retain some of the benefit the current code gets us. To summarize that, the discussion points, and other related topics here are some bullets:
I know none of the mitigating ideas or alternatives give us great options right now and some will likely add substantial effort. In the perfect world where available effort isn't a hindrance I'm drawn to the option of dynamic blocks that are all marked by the same unique block id which would instruct rendering to fetch details from an entity associated with that block position (which I think might already be the case with block type entities?). You'd end up with one entity per unique new dynamic block, which would probably hold a component with a list of all positions that block has been placed (or there'd be some other sort of cache somewhere) Partly I'm drawn to that option because it is an old idea - I've been shopping around the idea of "composite blocks" for years (see https://trello.com/c/zkMdfjDz/45-composite-blocks-multiparts - which was written long after the concept originally arose) without any relation to lazy loading. That concept would potentially both buy us new functionality and allow us to discard lazy loading of ids for all the benefits this PR brings us. STs that contain block combos lacking existing ids could probably just provision dynamic blocks on the fly. During game restart as the game parses its save file and modules it could both look for new block assets as well as any entities representing dynamic blocks and allocate them ids as well, then update the chunk files with the new ids, before starting up. That'd probably be a new phase of some sort that I suspect we'll need eventually anyway (imagine updating an existing world with new module versions but there are conflicts of some sort an upgrade tool would need to help with) Maybe we could even get to the point where a maintenance task could run live on a server and do a one-time refresh of the block id list to clients, rather than try to get lazy loading working everywhere, but I acknowledge that might just be tip-toeing back toward the original problem ... |
|
Heh, this is a tricky topic, and I'm commenting just based on the discussion (didn't look at the code yet). Some interesting aspect have already been brought up, some others are missing… To start off, I may need a glossary for the terminology used all around blocks. Maybe you can help me filling this in…
One thing I'm particularly worried about is how the migration after this change is going to work out. Who feels responsible for updating all structure templates, changing all modules, figuring out which blocks should be available in which shape, etc. …? I'd appreciate some small PoC, maybe with Lost and the STs defined in there. On the same note, I'm wondering about the best practices evolving from this. How are blocks defined, how are we going to use templates, and are we sure that delta prefabs and overrides are working correctly in all scenarios we can envision? What does the structure of block inheritance look like (e.g., 
Furthermore, do we have a way to define optional delta prefabs in some way? How can two modules that don't know about each other be combined? The figure below uses the cheese block from FunnyBlocks and the pillar shape from StructuralResoucres as example, but this could actually be anything. To avoid unwanted module dependencies I think we currently would need to introduce a now augmentation module for every combination we want to add 😕 This can easily lead to bloated module space if we're not careful here… 
The talk on extra data directly relates to untangling block rendering from block ids, doesn't it? I'd like to avoid custom solutions for some modules, but would rather like to take the opportunity to think about general solutions here. Varying the texture of a single block type has come up before (e.g., different grass path textures, or connected textures for larger areas of the same block type). Similarly, varying the shape depending on the block's context came up before (e.g., determining the exact shape of a fence or wire block, or connecting stairs placed around corners). If we work on some support here we should keep an eye on the full picture to fit everything in. (credit to @jdrueckert for the figures) |
One small correction: In many cases, the block family does not need to be explicitly specified. If the shape is rotationally symmetric (including the case where it's not explicitly defined and defaults to the cube shape), the family defaults to the SymmetricFamily, which contains only one block, and if the shape isn't symmetric (e.g. stairs), it defaults to the HorizontalFamily, which has 4 blocks: one for each horizontal rotation. |
|
One (or maybe two) more thing(s) coming to my mind is that having a bunch of new block definitions kind of requires a good naming scheme for them. We touched at that with some special blocks for HjerlHjede some time ago (camel case vs snake case vs ...). This may seem benign, but I think it's rather important to not end up with |
# Conflicts: # engine-tests/src/test/java/org/terasology/world/ChunkViewTest.java # engine-tests/src/test/java/org/terasology/world/generator/InternalLightGeneratorTest.java # engine-tests/src/test/java/org/terasology/world/propagation/BulkLightPropagationTest.java # engine/src/main/java/org/terasology/world/block/family/FreeformFamily.java
|
This pull request fixes 1 alert when merging 4441796 into 46bf9eb - view on LGTM.com fixed alerts:
|
skaldarnar
added
Breaking Change
Load every possible block family while the game is loading, rather than only when each block is requested from the
BlockManager. It isn't immediately obvious that this should be removed, but in my opinion it's more trouble than it was worth. Some of the main problems I have with the dynamic block assignment are that it's easy to miss that the block list may be incomplete (I missed it when writing FlowingLiquids for example), updating things can be complicated (there's a longstanding bug with registrations not propagating to clients properly in multiplayer), it just requires a lot of extra code to deal with dynamic restrations, and I vaguely remember people talking about a problem with including blocks that haven't yet been registered in UIs.In order to avoid having such a huge number of blocks that just loading them all takes an excessive amount of time, freeform block families have been restricted to only use shapes explicitly
marked as freeformUsable in their file. Currently only the cube, half-block, eighth-block and stair shapes are marked as freeformUsable. The engine also contains various slope shapes which look like they're probably intended for freeform use, but I've never seen them actually used and there are a lot of them, so I left them out.listed in the block file.This will probably require some module updates. In particular, any
BlockRegistrationListeners now no longer need to (or can) listen for block registrations, and if there are actual uses of any freeform block shapes other than the ones i marked, they will need to be explicitly listed in the block definition files.(Now already implemented)
It may become necessary to add some system to restrict freeform block families further if it turns out that there are a lot of shapes that are necessary in a few cases. From what I've seen though (mostly just Josharias Survival and Core actually), the wide range of options provided by the freeform block families aren't actually used much.Probably resolves #3491, but I still need to actually test that.