Skip to content
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: remove dependency on AnotherWorld #38

Merged
merged 6 commits into from
Aug 17, 2021

Conversation

naalit
Copy link
Contributor

@naalit naalit commented Aug 10, 2021

GrowingFlora currently depends on AnotherWorld, but there isn't much reason for that to be the case. I moved some utilities here and to the engine (see MovingBlocks/Terasology#4849, which this PR depends on) and updated some references, which was enough to remove the dependency. Behavior shouldn't change, except that it no longer considers parent biomes for AnotherWorldsBiomes, and it also no longer has a bug where branch angles were snapped to whole numbers of radians.

Eventually, it would be nice to remove the dependency on ClimateConditions (or hopefully make it optional so that climate still affects growth if it is present), since Metal Renegades would like to use GrowingFlora but doesn't necessarily want to enable ClimateConditions.

@naalit naalit changed the title chore: remove dependency on AnotherWorld refactor: remove dependency on AnotherWorld Aug 10, 2021
@naalit naalit added the Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity label Aug 10, 2021
skaldarnar
skaldarnar previously approved these changes Aug 17, 2021
Copy link
Contributor

@skaldarnar skaldarnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm this works with Terasology/MetalRenegades#161

float value = positionValue.getValue();

if (noise.noise(pos.x, pos.y, pos.z) / 256f < amount) {
facet.setFlag(pos, value);
if (foliageRegion.contains(pos) && noise.noise(pos.x(), pos.y(), pos.z()) / 256f < amount) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there no variant to pass a vector to noise.noise(pos)? If that is missing, we should add it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there isn't. Should that be added to the main Noise interface?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense, yes. We can use a default implementation for that to only touch that one file.

@@ -64,7 +65,7 @@ public void updatePlant(DelayedActionTriggeredEvent event, EntityRef plant, Livi
PerformanceMonitor.startActivity("GrowingFlora - Updating plant");
try {
PlantGrowthDefinition plantDefinition = plantRegistry.getPlantGrowthDefinition(plantComponent.type);
Long updateDelay = plantDefinition.requestedUpdatePlant(worldProvider, environmentSystem, blockEntityRegistry, plant);
Long updateDelay = plantDefinition.requestedUpdatePlant(worldProvider, new EnvironmentLocalParameters(environmentSystem, blockComponent.getPosition()), blockEntityRegistry, plant);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This whole system will only be initialized if all it's dependencies can be met. If ClimateConditions is not loaded, we'll see logs like the following, and the whole feature is not present.

22:47:03.042 [main] WARN  o.t.e.core.ComponentSystemManager - Skip system GrowingFlora:SaplingInitializeSystem for loading - possibly missing optional dependencies
22:47:03.045 [main] WARN  o.t.e.core.ComponentSystemManager - Skip system GrowingFlora:PlantGrowingSystem for loading - possibly missing optional dependencies

I think this is fine for now, but we can definitely improve on this, e.g., by basing those plant definition on something else if the environmentSystem is not present.

I think this is something for another PR, though. Maybe Terasology/SimpleFarming#120 can give some reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually fixed this right before this comment. Those systems are needed for plants to grow, so skipping them isn't great.

@skaldarnar skaldarnar merged commit 51ab1f4 into develop Aug 17, 2021
@skaldarnar skaldarnar deleted the chore/remove-another-world branch August 17, 2021 21:55
skaldarnar pushed a commit to Terasology/MetalRenegades that referenced this pull request Aug 18, 2021
Adds [juniper trees](https://en.wikipedia.org/wiki/Juniper) to steppes and [baobabs](https://en.wikipedia.org/wiki/Adansonia_digitata) to scrublands, using GrowingFlora and the assets from PlantPack.

Juniper trees are a type of cypress, so I'm using the cypress assets, and they do naturally appear in the American West; baobab trees actually live in Africa, but it's a similar environment and I think they make scrublands a lot more interesting, and baobab already exists in PlantPack unlike other tree species which would make more sense geographically.

Depends on Terasology/GrowingFlora#38. 

* feat: cypress/juniper and baobab trees with GrowingFlora
* fix: ground filters removed from GrowingFlora
* fix: planted saplings will actually grow
skaldarnar added a commit to Terasology/Alchemy that referenced this pull request Aug 19, 2021
skaldarnar added a commit to Terasology/Alchemy that referenced this pull request Aug 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Refactoring Request for or implementation of pure and automatic refactorings, e.g. renaming, to improve clarity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants