Fix gravity and entity-world collision for all entities - [merged] #144

Closed jdonkervliet closed 4 years ago

In GitLab by @julian9499 on May 19, 2020, 24:18

Merges feature/fix-gravity -> development

This merge request adds the feature for mobs to be affected by gravity and collide with blocks in the world. The gravity implementation was already present but is largely reworked to accommodate for the collision checking and response. The collision checking is achieved by using the swept-AABB method. This method also provides the collision response for the entity. Bounding box generation for blocks has been reworked to accommodate for different types of blocks as well.

In GitLab by @julian9499 on May 19, 2020, 24:19

added 22 commits

4691e4aa...40b62a4d - 21 commits from branch development
09804239 - fix merge conflicts

Compare with previous version

In GitLab by @swabbur on May 19, 2020, 01:53

Commented on src/main/java/net/glowstone/GlowWorld.java line 1652

The second whiteline here is redundant.

In GitLab by @swabbur on May 19, 2020, 01:54

Commented on src/main/java/net/glowstone/block/GlowBlock.java line 605

Do the fence and fence gate have the same bounding box?

In GitLab by @swabbur on May 19, 2020, 01:55

Commented on src/main/java/net/glowstone/block/GlowBlock.java line 596

Do all fence types listed below use the same boundingbox?

In GitLab by @swabbur on May 19, 2020, 01:56

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 227

Is there a specific reasoning behind these constants? Also, the comments do not match their values.

In GitLab by @swabbur on May 19, 2020, 01:58

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 364

We often attempt to prevent nesting or training calls. I don't find it especially annoying here, but for the sake of consistency, it may be better to rewrite it.

In GitLab by @swabbur on May 19, 2020, 01:59

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 523

Were all these changes made using auto-formatting? We discussed earlier that our styling doesn't correspond to that of Intellij one-to-one. So, it may have become more messy by doing so.

In GitLab by @swabbur on May 19, 2020, 02:01

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1018

Why create and iterate over a list like this? You are better of using something like:

List<BlockFace> faces = Arrays.listOf(
    BlockFace.NORTH,
    BlockFace.SOUTH,
    // ...
);

for (BlockFace face : faces) {
    // ...
}

Or even store the list somewhere else (looks an aweful lot like the existing ADJACENT and LAYER lists).

In GitLab by @swabbur on May 19, 2020, 02:03

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1075

These nested if-statements may need a little clarification. Adding some whitespace to the internals of the for-loop may also improve readability.

In GitLab by @swabbur on May 19, 2020, 02:05

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1090

It might be better to first compute the forced/acceleration to be used and applying them in a separate step at the end. It's quite confusing to follow what exactly is being done to the velocity.

In GitLab by @swabbur on May 19, 2020, 02:05

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1119

Don't forget to add a whiteline at the start of a multi-block method.

In GitLab by @swabbur on May 19, 2020, 02:06

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1154

I would advise you to use a different name for the "THETA" variable (and its local siblings) as it is a very commonly used an undescriptive name.

In GitLab by @swabbur on May 19, 2020, 02:06

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1177

Do we still need this comment here?

In GitLab by @swabbur on May 19, 2020, 02:07

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1754

Again, please verify whether these formatting changes were actually necessary and whether they clash with the existing code styling.

In GitLab by @swabbur on May 19, 2020, 02:09

Commented on src/main/java/net/glowstone/entity/GlowLivingEntity.java line 1034

Maybe the value 0.2 should be stored in a constant with a name describing its function. It would clarify what this method call is trying to achieve and would make it easier to adjust when needed.

In GitLab by @swabbur on May 19, 2020, 02:10

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 32

Why are these subtraction and addition still being performed while the later tolerance should already solve the problem?

In GitLab by @swabbur on May 19, 2020, 02:10

Commented on src/main/java/net/glowstone/entity/physics/BoundingBox.java line 18

Why is this method no longer final? Was it overriden in another class?

In GitLab by @swabbur on May 19, 2020, 02:11

Commented on src/main/java/net/glowstone/entity/physics/EntityBoundingBox.java line 29

Seems like the auto-formatting feature was used here as well. We've discussed JavaDoc spacing earlier and decided not to align these but keep them spaced with a single space from the parameter names instead.

In GitLab by @swabbur on May 19, 2020, 02:12

Commented on src/main/java/net/glowstone/entity/physics/EntityBoundingBox.java line 45

Could you rename the single-letter variables to full names (e.g. velocity instead of v)?

In GitLab by @swabbur on May 19, 2020, 02:13

Commented on src/main/java/net/glowstone/entity/physics/EntityBoundingBox.java line 60

We had earlier discussed to mostly use normal if-statements over ternary if-statements as they are more well-known and easier to understand for less experienced programmers. Also, keeping things consistent improves general code-quality and readability.

In GitLab by @swabbur on May 19, 2020, 02:13

Commented on src/main/java/net/glowstone/entity/physics/EntityBoundingBox.java line 73

Please, add a whiteline at the start of a multi-block method.

In GitLab by @swabbur on May 19, 2020, 02:14

Commented on src/main/java/net/glowstone/entity/physics/EntityBoundingBox.java line 140

Another whiteline should be added before this if-statement.

In GitLab by @swabbur on May 19, 2020, 02:15

Commented on src/main/java/net/glowstone/util/Vectors.java line 8

Could you add a private constructor to this class to prevent initialization?

In GitLab by @swabbur on May 19, 2020, 02:15

Commented on src/main/java/net/glowstone/util/Vectors.java line 21

JavaDoc is missing on each of these functions.

In GitLab by @swabbur on May 19, 2020, 02:15

Commented on src/test/java/net/glowstone/entity/GlowEntityTest.java line 17

If I remember correctly, we discussed not using any additional whitelines between imports (except between static and non-static ones).

In GitLab by @swabbur on May 19, 2020, 02:16

Commented on src/test/java/net/glowstone/entity/GlowEntityTest.java line 58

What is the lone paragraph element for?

In GitLab by @JimVliet on May 19, 2020, 09:13

Commented on src/main/java/net/glowstone/GlowWorld.java line 9

We should not use the star import here.

In GitLab by @JimVliet on May 19, 2020, 09:13

Commented on src/main/java/net/glowstone/GlowWorld.java line 60

Same here.

In GitLab by @JimVliet on May 19, 2020, 09:16

Commented on src/main/java/net/glowstone/block/GlowBlock.java line 598

The this. seems redundant here. A few lines below the isfence method is called without the this. too, so it would be best if you could do the same here.

In GitLab by @JimVliet on May 19, 2020, 09:41

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 986

I think that we should try to format only the code we are actively working on, not just the entire file. If we want to fix formatting issues, we should do so separately.

In GitLab by @JimVliet on May 19, 2020, 09:42

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1018

Agreed, this is kind of hard to read.

In GitLab by @JimVliet on May 19, 2020, 09:51

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1088

"that is will be..."

In GitLab by @wubero on May 19, 2020, 09:57

Commented on src/main/java/net/glowstone/GlowWorld.java line 9

This can be a star import in my opinion because you're importing so much.

In GitLab by @wubero on May 19, 2020, 09:58

Commented on src/main/java/net/glowstone/block/GlowBlock.java line 604

Should you not remove these comments?

In GitLab by @wubero on May 19, 2020, 09:58

Commented on src/main/java/net/glowstone/block/GlowBlock.java line 632

Missing javadoc

In GitLab by @wubero on May 19, 2020, 09:58

Commented on src/main/java/net/glowstone/block/GlowBlock.java line 665

Missing javadoc

In GitLab by @JimVliet on May 19, 2020, 09:59

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1100

There seem to be a lot of these random constants everywhere. Perhaps it might be better to declare them in one place, with a descriptive variable name.

In GitLab by @JimVliet on May 19, 2020, 09:59

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1100

Declaring them once, also makes it easier to modify them if need be.

In GitLab by @wubero on May 19, 2020, 10:01

Commented on src/main/java/net/glowstone/entity/physics/EntityBoundingBox.java line 71

Since this is such a large method it might serve us well to add comments within the methods so everyone can understand every part

In GitLab by @JimVliet on May 19, 2020, 10:08

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1123

Why does the elapsed time need to be less than 1? It is not exactly clear what unit this has; seconds, milliseconds?

In GitLab by @JimVliet on May 19, 2020, 10:14

Commented on src/main/java/net/glowstone/entity/GlowEntity.java line 1166

Is it not possible to implement this with an interface or something similar?