feat: add priority to Updates annotation of facet providers #4833
Conversation
github-actions
bot
added
the
Type: Improvement
naalit
added
the
Topic: WorldGen
| public static final int PRIORITY_PRODUCES = 250; | ||
| public static final int PRIORITY_CRITICAL = 200; | ||
| public static final int PRIORITY_HIGH = 150; | ||
| public static final int PRIORITY_NORMAL = 100; | ||
| public static final int PRIORITY_LOW = 50; | ||
| public static final int PRIORITY_TRIVIAL = 0; | ||
| public static final int PRIORITY_REQUIRES = -50; |
There was a problem hiding this comment.
I'm just doing the same thing as EventPriority, I'm not sure why that's done that way.
There was a problem hiding this comment.
I'm pretty sure we're not using an enum for event priority because the main goal here is to resolve non-determinism due to same priority. In many cases these default threshold already help to sort out whether something should run with low or high priority, but we may need the fine-grained support to run a NORMAL + 10 (slightly earlier than the rest of normal prio, but after high prio).
Given that, I'm wondering whether the defaults for PRODUCES and REQUIRES are good as they are here, or whether they should be way higher/lower (maybe even Integer.MINandInteger.MAX`).
There was a problem hiding this comment.
Given that, I'm wondering whether the defaults for PRODUCES and REQUIRES are good as they are here, or whether they should be way higher/lower (maybe even Integer.MINandInteger.MAX`).
I think that makes sense for PRODUCES, since it's impossible for anything to update the facet before it's produced. But some things actually want to run after REQUIRES, specifically the SpawnPlateauProvider from CoreWorlds, although those conflicts can often also be fixed by changing the Requires to Updates on other providers (SimplexRoughnessProvider in this case would pretend to update ElevationFacet).
There was a problem hiding this comment.
That totally makes sense to me. Is it possible to annotate the priority in @Requires as well?
There was a problem hiding this comment.
No, the idea is that if you need fine-grained control over when something reads the facet, you should just use @Updates. Of course, that is somewhat inconsistent with being able to @Update after @Require-ing like I did with the SpawnPlateauProvider, but I'm not sure what the best solution would be - it feels like any way of doing this is bad for modularity.
| public static String priorityString(int priority) { | ||
| switch (priority) { | ||
| case PRIORITY_PRODUCES: | ||
| return "PRIORITY_PRODUCES"; | ||
| case PRIORITY_CRITICAL: | ||
| return "PRIORITY_CRITICAL"; | ||
| case PRIORITY_HIGH: | ||
| return "PRIORITY_HIGH"; | ||
| case PRIORITY_NORMAL: | ||
| return "PRIORITY_NORMAL"; | ||
| case PRIORITY_LOW: | ||
| return "PRIORITY_LOW"; | ||
| case PRIORITY_REQUIRES: | ||
| return "PRIORITY_REQUIRES"; | ||
| default: | ||
| return Integer.toString(priority); | ||
| } |
There was a problem hiding this comment.
Should this add the numeric value in parentheses behind the human readable name? This might help to bring this in proportion to custom priorities if they ever appear close to each other in a log.
There was a problem hiding this comment.
you can use them like ranges
if priority < a
else if priority < b
else if priority < c
else if priority < d
else if priority < e
PRIORITY_(value)
| return; | ||
| } | ||
| Stream.of(updatedFacets(provider), requiredFacets(provider)).flatMap(Arrays::stream).forEachOrdered(r -> { | ||
| if (r.value() != providedFacet) { |
There was a problem hiding this comment.
Is that one big if around the rest, with no else? If so, I'd move this out of the forEach and use a .filter(r -> r.value() == providedFacet) for that.
| private void addProviderChain(Class<? extends WorldFacet> facet, boolean scalable, int minPriority, | ||
| Set<FacetProvider> orderedProviders) { |
There was a problem hiding this comment.
Could you please provide some Javadoc what this method is doing (could also be a general follow-up PR to add docstrings if you prefer). I few things I noticed from just looking at the PR
orderedProvidersis a "destination" parameter and is populated in this methodprovidedByis updated- there is a "hidden" recursion via
addRequirements
skaldarnar
changed the title
This fixes the circular dependency which is recognized by MovingBlocks/Terasology#4833, which is the only change needed to make it work again.
Contains
WorldBuildercomes up with the order to run facet providers in based on their annotations -@Requires,@Produces, and@Updates. But right now, there's no way to control order between@Updatesproviders - for example, a river provider should update the elevation after a mountain provider, so that rivers are carved into mountains and not the other way around. This PR adds a mechanism to do that, another field on@Updates(value = ..., priority = UpdatePriority.PRIORITY_HIGH)with similar effect to the existing event handler priority mechanism. This also ended up simplifying the provider ordering code.There's now a circular dependency in
CoreWorlds, sinceSpawnPlateauProviderrequiresSurfaceRoughnessFacetandSimplexRoughnessProviderrequiresElevationFacet, which can be resolved by explicitly givingSpawnPlateauProvidera lower priority - there's even a nice error message that suggests the correct change. Required changes to other world generators should be similarly minor and straightforward.