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

Remove unnecessary checks in IslandsManager and IslandWorldManager. #692

Merged
merged 1 commit into from
May 23, 2019

Conversation

BONNe
Copy link
Member

@BONNe BONNe commented May 20, 2019

Removed checks overlaps a lot. It is not necessary to check multiple time if island is nether and end island, as they will not change their status in runtime.

THIS WILL BREAK SOME TESTS.
But not because changes are not correct, but because tests are incorrect. They set #inWorld(World) to always return true.

Found this by investigating #676.

getIslandAt(Location) optimization is correct, as inWorld(World) already checked all these options that is required for world to be an Island World.

inWorld(World) can be optimized even more, by creating local object that contains worldSettings, to avoid getting element from map in isIslandNether(World) and isIslandEnd(World).

Removed checks overlaps a lot. It is not necessary to check multiple time if island is nether and end island, as they will not change their status in runtime.
@Poslovitch Poslovitch added Status: Need review Waiting for a review to be made on this Pull Request Type: Enhancement Improvement or modification which is usually a new feature. labels May 21, 2019
@Poslovitch Poslovitch self-requested a review May 21, 2019 04:22
@Poslovitch Poslovitch added this to the 1.5.0 milestone May 21, 2019
@tastybento tastybento self-requested a review May 22, 2019 22:59
@tastybento
Copy link
Member

Thanks @BONNe . These changes make sense. The history is that the World/GameModes map originally only held the normal world and not the nether and end, so that's why there were these convoluted checks. Then we moved to storing all three of the worlds in the map but did not change the checking code.

Copy link
Member

@tastybento tastybento left a comment

Choose a reason for hiding this comment

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

If this PR is merged, then the tests will need fixing. Assign that to me.

@tastybento tastybento self-assigned this May 22, 2019
Copy link
Member

@Poslovitch Poslovitch 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 as well. I'll merge the PR.

@Poslovitch Poslovitch added Status: Done This issue has been completed or answered. This pull request has been merged. and removed Status: Need review Waiting for a review to be made on this Pull Request labels May 23, 2019
@Poslovitch Poslovitch merged commit 61a5b69 into BentoBoxWorld:develop May 23, 2019
tastybento added a commit that referenced this pull request May 24, 2019
Relates to #692
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Done This issue has been completed or answered. This pull request has been merged. Type: Enhancement Improvement or modification which is usually a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants