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

Update Dandelifeon to respect enchanted soil #4660

Open
wants to merge 8 commits into
base: 1.20.x
Choose a base branch
from

Conversation

NEstoll
Copy link
Contributor

@NEstoll NEstoll commented May 28, 2024

Fixes #4405

@TheRealWormbo
Copy link
Collaborator

I feel like the enchanted soil status should probably have an effect on the boundary cell check somehow, but I'm not sure if the adjacent flower also needs to be on enchanted soil, or if otherwise boundary cells from non-boosted flowers should just be ignored.

Copy link
Collaborator

@TheRealWormbo TheRealWormbo left a comment

Choose a reason for hiding this comment

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

See code comments. Functionality-wise it looks good to me, though.

@NEstoll NEstoll requested a review from TheRealWormbo June 19, 2024 19:37
Copy link
Collaborator

@TheRealWormbo TheRealWormbo left a comment

Choose a reason for hiding this comment

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

See comment in code. Also, please rebase onto current 1.20.x branch head to fix the dependency issues with VanillaGradle.

@NEstoll NEstoll requested a review from TheRealWormbo July 10, 2024 18:14
@NEstoll NEstoll force-pushed the Dandelifeon_overgrowth branch from 8633982 to ae52012 Compare July 10, 2024 18:52
@NEstoll NEstoll force-pushed the Dandelifeon_overgrowth branch from c21f015 to d43da4c Compare July 10, 2024 20:36
Copy link
Collaborator

@TheRealWormbo TheRealWormbo left a comment

Choose a reason for hiding this comment

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

Looks good to me.

public boolean hasPoweredParent(Level level) {
return flowerCoords != null && level.getBlockEntity(flowerCoords) instanceof DandelifeonBlockEntity && level.hasNeighborSignal(flowerCoords);
public boolean hasActiveParent(DandelifeonBlockEntity dandie) {
return flowerCoords != null && dandie.getLevel().getBlockEntity(flowerCoords) instanceof DandelifeonBlockEntity parent && dandie.getLevel().hasNeighborSignal(flowerCoords) && (!dandie.overgrowthBoost || parent.isOnSpecialSoil());
Copy link
Member

Choose a reason for hiding this comment

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

This or-check at the end is weird to me. It's true if the flower it's being transferred to is not boosted, or if the current flower is boosted. (Did i get that right?) So this returns true when both are boosted or neither are boosted, and false if the old flower is not boosted and the new one is. But if the old flower is boosted and the new one is not, this will still return true, right? Which we don't want, right?

It would also be nice if these variable names better reflected which flower it's coming from and which flower it's going to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is always called by the new flower, so we only need to make sure that either this is a normal tick (we aren't in an overgrowth tick), or that the other flower is also enchanted, and therefor will tick as well. This means that adjacent flowers will only share cells on ticks where they both are ticking. If the old flower is boosted and the new one isn't, then the new one won't be ticked, so this will never be called, and thus the return value doesn't matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

enchanted soil does not accelerate Dandelifeon
3 participants