-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(JOML): Migrate methods of WorldProvider #3899
feat(JOML): Migrate methods of WorldProvider #3899
Conversation
This pull request introduces 1 alert when merging 1f51efb into 12f4964 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 220b894 into 12f4964 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 2f4abaa into 12f4964 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick review, haven't tested - minor comments. Also does this replace / make obsolete #3837?
@@ -269,6 +269,16 @@ public void testEntityBecomesTemporaryWhenChangedFromAKeepActiveBlock() { | |||
assertFalse(blockEntity.isActive()); | |||
} | |||
|
|||
@Test | |||
public void testEntityBecomesTemporaryWhenChangedFromAKeepActiveBlockJoml() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this test name 👍
* @param pos The position | ||
* @return Whether the given block is active | ||
*/ | ||
boolean isBlockRelevant(Vector3ic pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider (in general) adding default interface implementations when adding new methods like this? That could avoid having to update some things in modulespace. In this case I'm not sure it makes sense as I only see one non-engine usage over in FlexiblePathfinding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't hurt at the moment but a lot of this is temporary and all the old methods are going to go away anyways. It can't hurt to implement it even if the use case is limited.
@Override | ||
public boolean isBlockRelevant(Vector3f pos) { | ||
return isBlockRelevant(new Vector3i(pos, RoundingMode.HALF_UP)); | ||
} | ||
|
||
@Override | ||
public boolean isBlockRelevant(Vector3fc pos) { | ||
return isBlockRelevant(new org.joml.Vector3i(pos, org.joml.RoundingMode.HALF_UP)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explicit package like this makes me sad - can we avoid it via imports or is something in the way of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, Vector3i conflicts with Vector3i from termath. I expect this to be temporary until we get to the point of removing all the old methods. we can use an alias but that somehow feels worse.
I will start going through some of the more major systems in Terasology and provide JOML alternatives that we can start to use in place of the teramath implementations. we should be able to start removing the termath methods when these endpoints get updated in module space. I think I will start to do this for some of the more major systems that terasology uses to make the transition easier. Components are another issue that will be harder to address.
These methods are now backed by their joml equivalent so any actions on a given block should be the same from before. I expect breaking blocks and placement of blocks should still work the same regardless.