FacetProvider requirements should be verified

[keturn]

For example, BiomeProvider @Requires SeaLevelFacet, but code reaches BiomeProvider.process without a SeaLevelFacet provided: https://scans.gradle.com/s/3szw7vgxv4d2o/tests/:modules:DynamicCities:test/org.terasology.dynamicCities.settlements.SettlementEntityManagerTest/placeParcel(SettlementEntityManager,%20ModuleTestingHelper)?top-execution=1

If this had been checked up-front and responded with a clear error message, it would have saved me some time in trying to figure out why my WorldGenerator wasn't working.

@tolziplohu says:

Huh. It looks like the annotations are used for ordering providers but not for verifying that they actually exist. That should actually be pretty easy to detect — just look for empty provider lists for required facets. I guess we just haven't considered that possibility.

[Cervator]

Hi @zhayun3233 - https://discord.gg/terasology is the place for various general support :-)

If you're curious about this topic in particular check out https://github.com/Terasology/TutorialWorldGeneration and our getting started docs in this repo's wiki 👍

[MrGizmo123]

Would the WorldBuilder class (org.terasology.engine.world.generation.WorldBuilder) be a good place to perform the check if all required facet providers are available? More specifically in the determineProviderChains function where the other annotations are processed (line 178)

[jdrueckert]

Adding a link here for easier access: https://github.com/MovingBlocks/Terasology/blob/develop/engine/src/main/java/org/terasology/engine/world/generation/WorldBuilder.java#L177-L188

@MrGizmo123 Without having actively looked for alternative places, this seems like a good place to add such a check as we have easy access to both, the list of already added ("present") facets and the facet to be added to the chain.

For me the question would be, what is the expected behavior if we notice that a required facet is not yet present in the chain? Should we only log something or fail entirely? Personally, I think I would favor the latter to get early feedback when developing, but I'm not sure whether I'm overlooking scenarios in which this behavior would not be favorable... what do you think?
I'd also be interested in your opinion @keturn as you ran into it and requested this improvement 🙂

[MrGizmo123]

@jdrueckert I agree, as in I think it should fail entirely since its going to crash anyway a few moments later. I think the only time this error will come up is during development of a new world generator and so a prominent error message (after failure) will be most helpful.

[MrGizmo123]

An if statement like this should be enough https://github.com/MrGizmo123/Terasology/blob/develop/engine/src/main/java/org/terasology/engine/world/generation/WorldBuilder.java#L230-L233. I'm also not sure how to exit the program properly.