feat: improve 'playerHeight' command #3841

godfather2 · 2020-03-03T13:30:36Z

Contains

Attempt to fix #2535 .
The character's jump speed,movement speed,interaction range and step height are a function of the player's height.I acquired the functions through play testing and graph plotters.
Earlier, a change in the player's height was made through a change in the his position through teleportation.This is probably what caused the player to sink through blocks at dwarf heights.This pull request, on the other hand just changes the eye height of the player.

Increase/Decrease jump speed according to size. Giants should be able to jump higher and dwarfs lower.
Giants should take longer strides, increase runFactor?
Unable to pick up blocks off ground when height > 10. Bug!
Physics bug: Eye height translates gazeMountPoint to a value more than expected.
Trickier: Enable giants to be able to automatically climb up (no jump) blocks of smaller elevation.
Height too small, makes you fall through blocks.
"Increase/Decrease damage from fall. (30m giant should not die of a 15m fall). Define function/proportionality constant same as above."->this sub issue is dealt by a pull request in the health module

How to test

Just change the player's height and his capabilities should increase or decrease accordingly.

Outstanding before merging

Potions should use a AffectHeightComponent instead of affecting the PlayerConfig settings. These should override the Default (config) values and should be serialized to the world save.

GooeyHub · 2020-03-03T13:30:37Z

Can one of the admins please verify this patch?

syntaxi

Initial style concerns.
I'll do a proper review soon

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar

@godfather2 thanks for this PR.

Would be nice if you could come back to this once the proposal deadline is over.

There are some style guide violations in this PR. Resolving them should also help with making the code more readable and self-documenting (better names).

Potions should use a AffectHeightComponent instead of affecting the PlayerConfig settings. These should override the Default (config) values and should be serialized to the world save.

Is that item really outstanding before merging or could it be done as a follow-up?

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar · 2020-03-30T20:45:55Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

@@ -233,22 +231,44 @@ public String stepHeight(@Sender EntityRef client, @CommandParam("height") float
        return "";
    }

+    private float function1(float parameter){
+        double p = (double)parameter;
+        return (float) pow(p,0.6) * 0.78f ;


What's the reason to cast between float and doubles here? Wouldn't the following work?

private double calculateSomething(double p) { return pow(p, 0.6) * 0.78; }

I guess this could even be inlined.

The values returned from the functions are assigned to float variables.Hence the cast

skaldarnar · 2020-03-30T20:46:31Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+        return (float) pow(p,0.6) * 0.78f ;
+    }
+
+    private float function2(float parameter){


Same here, please use a better name for the method.

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar · 2020-03-30T20:49:22Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+            GazeMountPointComponent gazeMountPointComponent = player.getComponent(GazeMountPointComponent.class);
+
+            float ratio = amount / 1.6f;
+            float constant= function1(amount);


What constant is calculated based off the height (amount)? Is it the eye level or something like that?

skaldarnar · 2020-03-30T20:52:23Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+                move.jumpSpeed = 12 * constant;
+                move.stepHeight = 0.35f * ratio;
+                move.distanceBetweenFootsteps = ratio;
+                move.runFactor = function2(amount);


If function2 computes the run factor (whatever that is) you could call it getRunFactor instead.

…ommands.java Co-Authored-By: Tobias Nett <[email protected]>

godfather2 · 2020-04-02T07:10:03Z

@skaldarnar "Potions should use a AffectHeightComponent instead of affecting the PlayerConfig settings. These should override the Default (config) values and should be serialized to the world save."
This is actually a follow up.I only mentioned it because it was mentioned as one of the extras in the issue

skaldarnar

The changes are much more comprehensible now with better naems for the functions and variables.

One question, though: where did you get the formulas from? They don't match the default values, unfortunately, which I think would be a requirement to meet. Should also be possible to write tests for.

skaldarnar · 2020-04-02T15:47:31Z

engine/src/main/java/org/terasology/logic/characters/CharacterMovementComponent.java

@@ -91,6 +91,8 @@ public Vector3f getVelocity() {
        return velocity;
    }

+    public float getPlayerHeight(){ return height; }


The member height is public in CharacterMovementComponent, so there should be no need for this getter method.

See CharacterMovementComponent.java#L39

skaldarnar · 2020-04-02T15:53:39Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+            EntityRef player = clientComp.character;
+            GazeMountPointComponent gazeMountPointComponent = player.getComponent(GazeMountPointComponent.class);
+
+            float ratio = amount / 1.6f;


I believe 1.6f is the default height of the player? Ideally, we don't have to copy around these magic numbers and loose track of where they came from.

The best shot could be to define constants in CharacterMovementComponent which can be reused here.

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar · 2020-04-02T16:36:52Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

@@ -233,22 +236,48 @@ public String stepHeight(@Sender EntityRef client, @CommandParam("height") float
        return "";
    }

+    private float getJumpSpeed(float amount){
+        double d = (double)amount;
+        return (float) pow(d,0.6) * 9.36f ;


The default jumpSpeed seems to be 10 (and that's the maximum at the same time o.O ). thus, I would assume that setting the player height to the default should result in the default jump speed, but substituting amount by 1.6 yiels a value ~12.4 (≠10).

This is how the jump speed is adjusted according to this function, for amount in the interval from 0 to 5:

see Wolfram Alpha

see CharacterMovementComponent.java#L53-L54

skaldarnar · 2020-04-02T16:42:16Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+    }
+
+    private float getRunFactor(float amount){
+       return 0.15f + (0.64f * amount) - (0.006f * amount * amount);


The run factor seems to be a value in the range from 0 to 10, with 1.5 being the default.

Again, calculating the new run factor with the default height of 1.6 yields a value of 1.15864 (≠1.5).

This is how the run factor scales with the given function for amount between 0 and 5. The curve looks almost linear, to be hones:

see Wolfram Alpha

skaldarnar · 2020-04-02T16:56:27Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+
+    private float getInteractionRange(float amount){
+        double d = (double)amount;
+        return (float) pow(d,0.6) * 3.9f ;


The default interaction range seems to be 5 (see CharacterComponent.java#L37). With this formula the interaction range for height of 1.6 is 5.17055 (≠ 5).

This is what the plot looks like:

see Wolfram Alpha

lgtm-com · 2020-04-04T16:59:50Z

This pull request introduces 1 alert when merging fa8d3ed into 0579738 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

skaldarnar

Note that in all the suggested formulas I've use height/DEFAULT_HEIGHT (which is exactly the ratio you've computed before). This is in order to get that term to equal 1 for the default height, and thus result in the default value.

All the functions should get the ratio as an argument, and not the height.

skaldarnar · 2020-04-04T17:43:54Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+            float defaultHeight = move.height;
+            float defaultStepHeight = move.stepHeight;
+            float defaultFootSteps = move.distanceBetweenFootsteps;


This does not take the default values, but the current values for the CharacterMovementComponent.

skaldarnar · 2020-04-04T18:04:45Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+    private float getJumpSpeed(float amount){
+        double d = (double)amount;
+        return Math.round(pow(d,0.55) * 8.45f);
+    }


Suggested change

private float getJumpSpeed(float amount){

double d = (double)amount;

return Math.round(pow(d,0.55) * 8.45f);

}

private float getJumpSpeed(float height) {

return Math.round(Math.pow(height/DEFAULT_HEIGHT, 0.55f) * DEFAULT_JUMP_SPEED);

}

skaldarnar · 2020-04-04T18:06:44Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+    private float getInteractionRange(float amount){
+        double d = (double)amount;
+        return Math.round(pow(d,0.6) * 3.9f);
+    }


Suggested change

private float getInteractionRange(float amount){

double d = (double)amount;

return Math.round(pow(d,0.6) * 3.9f);

}

private float getInteractionRange(float height){

return Math.round(pow(height/DEFAULT_HEIGHT, 0.6) * DEFAULT_INTERACTION_RANGE);

}

skaldarnar · 2020-04-04T18:14:05Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+    private float getRunFactor(float amount){
+       return Math.round(5.5f + (6.4f * amount) - (0.06f * amount * amount)) / 10f;
+    }


Maybe use a similar formular to the ones you use above? Is there any reason this should be quadratic?

Suggested change

private float getRunFactor(float amount){

return Math.round(5.5f + (6.4f * amount) - (0.06f * amount * amount)) / 10f;

}

private float getRunFactor(float height){

return Math.round(Math.pow(height/DEFAULT_HEIGHT, 0.5) * DEFAULT_RUN_FACTOR);;

}

well....its just that i painstakingly constructing the other 2 functions manually,before i realized that it is much easier to arrive at them through regression,and that is how i acquired the third function.I must admit @skaldarnar you are really thorough

skaldarnar · 2020-04-04T18:15:20Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+            EntityRef player = clientComp.character;
+            GazeMountPointComponent gazeMountPointComponent = player.getComponent(GazeMountPointComponent.class);
+
+            float defaultHeight = 1.6f;


These should not be re-defined here. When the default values in CharacterMovementComponent are changed these will be outdated, and will likely cause some weird behavior and bugs.

Instead, CharacterMovementComponent should expose

public static final float DEFAULT_HEIGHT = 1.6f; public static final float DEFAULT_JUMP_SPEED = 10.f;

i just looked at the restoreSpeed command. This is how it accesses default values CharacterMovementComponent moveDefault = prefab.get().getComponent(CharacterMovementComponent.class); for instance move.jumpSpeed = moveDefault.jumpSpeed; Should i do this?

engine/src/main/java/org/terasology/logic/characters/CharacterMovementComponent.java

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar · 2020-04-19T10:14:19Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+                Optional<Prefab> prefab = Assets.get(new ResourceUrn("engine:player"), Prefab.class);
+                CharacterMovementComponent moveDefault = prefab.get().getComponent(CharacterMovementComponent.class);
+
+                if (move != null && gazeMountPointComponent != null) {


Ideally we'll need a lot more null checks here (for all the components). I'll refactor that part after this PR is merged, though.

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar · 2020-04-19T10:16:48Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+                    gazeMountPointComponent.translate.y = amount - foreHeadSize;
+                    Location.removeChild(player, gazeMountPointComponent.gazeEntity);
+                    Location.attachChild(player, gazeMountPointComponent.gazeEntity, gazeMountPointComponent.translate, new Quat4f(Quat4f.IDENTITY));
+
+                    player.saveComponent(gazeMountPointComponent);


As this is the same code as for the playerEyeHeight command I would refactor this later on to use a common

updateGazeMountPointHeight(player, amount - foreHeadSize);

skaldarnar · 2020-04-19T10:17:40Z

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

+                    return "Height of player set to " + amount + " (was " + prevHeight + ")";
+                }
+                return "";
+            } catch (NullPointerException e) {


If we did proper null checks this try-catch block should not be necessary (in general, catching a NPE is always a code smell as you could have done a null-check before).

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java

skaldarnar · 2020-04-19T11:05:53Z

I think that this command should, under the hood, use a ScaleCharacterEvent such that other systems can also react on it (e.g., the Health system also needs to adjust values on height change).

Merging this now as clear improvement and good progress to get the logic right.

Changes have been addressed.

implements height change extras and fixes bugs

5212bad

godfather2 mentioned this pull request Mar 3, 2020

Height extras Terasology/Health#36

Closed

1 task

syntaxi previously requested changes Mar 20, 2020

View reviewed changes

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java Outdated Show resolved Hide resolved

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java Outdated Show resolved Hide resolved

syntaxi added the Type: Improvement Request for or addition/enhancement of a feature label Mar 20, 2020

skaldarnar requested changes Mar 30, 2020

View reviewed changes

godfather2 and others added 4 commits April 2, 2020 11:27

Expanded star import

f94718f

Update engine/src/main/java/org/terasology/logic/debug/MovementDebugC…

26f8f98

…ommands.java Co-Authored-By: Tobias Nett <[email protected]>

Assigned more meaningful names and cleaned up code

1e21cc6

Fixed small mistake

6b915ef

skaldarnar requested changes Apr 2, 2020

View reviewed changes

godfather2 added 3 commits April 4, 2020 12:57

Got rid of unnecessary getter

87596cf

Made necessary changes

851d6b6

Changed the height range

fa8d3ed

corrected small mistake

944bc1e

skaldarnar requested changes Apr 4, 2020

View reviewed changes

godfather2 added 3 commits April 4, 2020 23:54

Update MovementDebugCommands.java

9de2a0b

Cleaned up formulae

f29f784

Corrected the default height and height range

0b002b9

skaldarnar approved these changes Apr 19, 2020

View reviewed changes

Apply suggestions from code review

cb8015c

skaldarnar reviewed Apr 19, 2020

View reviewed changes

engine/src/main/java/org/terasology/logic/debug/MovementDebugCommands.java Outdated Show resolved Hide resolved

added missed ';'

3acd387

skaldarnar changed the title ~~Height change extras~~ feat: improve 'playerHeight' command Apr 19, 2020

skaldarnar merged commit f7d3de0 into MovingBlocks:develop Apr 19, 2020

skaldarnar mentioned this pull request Apr 19, 2020

Scaling the player leaves inconsistent state #2535

Open

11 tasks

Cervator added this to the v3.0.1 milestone Apr 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: improve 'playerHeight' command #3841

feat: improve 'playerHeight' command #3841

godfather2 commented Mar 3, 2020 •

edited

Loading

GooeyHub commented Mar 3, 2020

syntaxi left a comment

skaldarnar left a comment

skaldarnar Mar 30, 2020

godfather2 Apr 2, 2020

skaldarnar Mar 30, 2020

skaldarnar Mar 30, 2020

skaldarnar Mar 30, 2020

godfather2 commented Apr 2, 2020

skaldarnar left a comment

skaldarnar Apr 2, 2020

skaldarnar Apr 2, 2020

skaldarnar Apr 2, 2020

skaldarnar Apr 2, 2020

skaldarnar Apr 2, 2020

lgtm-com bot commented Apr 4, 2020

skaldarnar left a comment

skaldarnar Apr 4, 2020

skaldarnar Apr 4, 2020

skaldarnar Apr 4, 2020

skaldarnar Apr 4, 2020

godfather2 Apr 4, 2020

skaldarnar Apr 4, 2020

godfather2 Apr 4, 2020

skaldarnar Apr 19, 2020

skaldarnar Apr 19, 2020

skaldarnar Apr 19, 2020

skaldarnar commented Apr 19, 2020

feat: improve 'playerHeight' command #3841

feat: improve 'playerHeight' command #3841

Conversation

godfather2 commented Mar 3, 2020 • edited Loading

Contains

How to test

Outstanding before merging

GooeyHub commented Mar 3, 2020

syntaxi left a comment

Choose a reason for hiding this comment

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

godfather2 commented Apr 2, 2020

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 4, 2020

skaldarnar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skaldarnar commented Apr 19, 2020

godfather2 commented Mar 3, 2020 •

edited

Loading