MachineMuse / MachineMusePowersuits

Minecraft mod, take 2
236 stars 105 forks source link

Kinetic generator jumping #896

Closed ghost closed 5 years ago

ghost commented 5 years ago

If the kinetic generator is set anything above the bare minimum, it causes momentum loss that is disproportionate to the amount of power generated, and is not in line with the movement resistance indicated.

Forge: 2800 Powersuits: 0.7.0.029 Numina: 1.10.0

default config

lehjr commented 5 years ago

The kinetic generator calculations are here if you want to take a look: https://github.com/MachineMuse/MachineMusePowersuits/blob/1.12.2-experimental/src/main/java/net/machinemuse/powersuits/powermodule/energy_generation/KineticGeneratorModule.java#L46-L61

The reality is that it isn't meant to be a top tier charging solution or limitless source of power with little to no tradeoff, or charge faster than the sprint boost module consumes energy like in previous versions. In 1.13 and beyond, power generation itself will likely be removed.

ghost commented 5 years ago

I understand that, but going from 2.00k rf/t with no movement penalty to 2.04k rf/t with a massive penalty doesn't seem right. The movement penalty also doesn't seem to change between moving it slightly off minimum and maxing it out. This is also irregardless of the sprint boost module settings.

lehjr commented 5 years ago

energy is only applied per 20 ticks.

lehjr commented 5 years ago

This is what the energy generated looks like at the 2 ends of the slider values.

at slider at 0: [09:38:42] [Server thread/INFO] [STDOUT]: [net.machinemuse.powersuits.powermodule.energy_generation.KineticGeneratorModule:onPlayerTickActive:62]: Movement Resistance: 0.0 [09:38:42] [Server thread/INFO] [STDOUT]: [net.machinemuse.powersuits.powermodule.energy_generation.KineticGeneratorModule:onPlayerTickActive:63]: Energy generated: 3367

with slider at full: [09:40:01] [Server thread/INFO] [STDOUT]: [net.machinemuse.powersuits.powermodule.energy_generation.KineticGeneratorModule:onPlayerTickActive:62]: Movement Resistance: 0.5 [09:40:01] [Server thread/INFO] [STDOUT]: [net.machinemuse.powersuits.powermodule.energy_generation.KineticGeneratorModule:onPlayerTickActive:63]: Energy generated: 8410

And keep in mind that this is calculated per distance traveled. As the player slows down, the distance per tick will be less. Is it ideal, no. It's not meant to be. Generator output isn't directly proportional to the amount of kinetic energy you put in.

ghost commented 5 years ago

The problem is that the movement resistance indicated is not what is actually happening. In reality it's closer to 95-98%, and jumping over a gap is near impossible, even with Sprint assist.

lehjr commented 5 years ago

Again, it is not meant to be used as an end game charging solution. This is also why you have the ability to turn it off.

            player.motionX *= movementResistance;
            player.motionZ *= movementResistance;

That is the actual code. And as I showed you above, at max setting, the value is 0.5 Also, using sprint assist while using the kinetic energy generator is ... well let's just say "perpetual motion" went out with the last version.

ghost commented 5 years ago

I understand this.

I also understand that I cannot jump over even a 1 block gap with the kinetic generator set to even 1 tick above minimum (a 1% movement speed reduction, or so it indicates).

Not to be rude, but this is going in circles. What the console, code mentioned, and table say is NOT what is actually going on. If I set the generator to minimum, everything is fine, including jumping over gaps; if I set the generator to even the slightest bit more, my movement speed falls off a cliff, and jumping speed even more. I'll attach a video later on showing this when I have more time available.

lehjr commented 5 years ago

I gave you the numbers, I showed you the code. I'm not sure how much more explaining I can do. At the lowest setting, there is no movement resistance, as indicated. Once you move the slider up from there, movement resistance is applied. As movement resistance increased, total output will be reduced because speed is reduced. This is how it is intended to function.

In fact, this is the actual distance traveled per tick, just sprinting: at slider = 0: 0.23572235107421874

at slider = full 0.14719696044921873

As you can see, the distance per time is actually a little more than half. So I'm not sure where you're getting that the speed decrease is over 90% or any of your other numbers.

ghost commented 5 years ago

Well that was rude

Here's the promised video link: https://www.dropbox.com/s/m8e6yvdsftl5f1c/Powersuits%20Bug.mkv?dl=0

I will reiterate, this is with just powersuits installed, default config.

eyeonus commented 5 years ago

CONFIRMED.

Tested with only MPS installed. Latest version, Forge 2808.

Takes ~2 seconds to walk 10 blocks, no modules active. Takes ~4 seconds to walk 10 blocks, kinetic genetor enabled and at max. Takes ~6 seconds to walk 10 blocks, kinetic generator enabled at 0.14%

Video to follow as soon as it's done uploading.

eyeonus commented 5 years ago

LOL, bad developer, you go time out! :D

ghost commented 5 years ago

LOL, bad developer, you go time out! :D

HAHAHAHAHAHAHAHAHA, I'm dying over here xD

eyeonus commented 5 years ago

He's the one that put "bad developer", so that proves at least that he's willing to (gracefully) admit when he's wrong.

eyeonus commented 5 years ago

Also, holy crap is sprint assist fast now. :)

lehjr commented 5 years ago

there are actually several issues here. The first one is that movement resistance should start at more than zero. The second issue is that player.motionX = movementResistance; player.motionZ = movementResistance;

should be player.motionX = (1- movementResistance); player.motionZ = (1 -movementResistance);

But, the code for the sprint assist module has to be adjusted to factor this in, since the speed is applied differently.

As for the sprint assist module: https://brightcoconut.com/wp-content/uploads/2018/02/ludicrous-speed.jpg

lehjr commented 5 years ago

eyeonus commented 5 years ago

Video:

https://youtu.be/VpBrFbD4kZM

ghost commented 5 years ago

-watches video- lmao yeah, that's why I had the settings I did at first

Also worthy of note, it really makes flying WONKY. Trying to fly with the kinetic generator feels like you had too many drinks, especially with flight control.

eyeonus commented 5 years ago

Any idea why jumping kills your speed so much?

(Also, I think you might want to look into the math on the sprint assist module, because the max setting says 200% = 2 vanilla, but it seems more like 200 vanilla. (Speaking specifically of the "Walking Speed Multiplier".))

ghost commented 5 years ago

I'm guessing it's the same issue causing the speed, jumping, and flying issues. In minecraft you do lose momentum while jumping, this is just exaggerating it to the extreme.

lehjr commented 5 years ago

the main issue is using player.motionX and player.motionY in the first place. The right way is to use the attribute modifier instead. Unfortunately, it's more complicated than that because the sprint assist is also using that.

eyeonus commented 5 years ago

And I'm guessing having both modules do it "the right way" would be difficult.

lehjr commented 5 years ago

which is why i really didn't want to implement the kinetic generator in 1.12.2 in the first place. It's not impossible, but would be messy.

lehjr commented 5 years ago

I have an idea... it will be a bit messy, but it should work

ghost commented 5 years ago

Since from what I can tell 1.13 is going to force a bunch of rewriting of things, it might be a good opportunity for a cleanup.

eyeonus commented 5 years ago

I'm guessing the "right way" is this line?

setMovementModifier(item, walkMultiplier);

If so, I can see why it would be complicated and messy. You'd have to make all the movement speed modifying modules aware of each other. Ugh.

lehjr commented 5 years ago

sort of, but remember that the tick loop is running (onPlayerTickActive/onPlayerTickInActive) so it's not as simple as that.

lehjr commented 5 years ago

cleanup isn't really the issue, it's more that some things aren't possible to implement without being a bit hacky.

ghost commented 5 years ago

Ah ok. I'm not that into coding, my depth is enough to be able to report bugs to be fixed by those that are.

lehjr commented 5 years ago

1.13 so far is quite a bit different. I'm not even sure I'd be able to bundle Numina with MPS in the same jar if I wanted to.

ghost commented 5 years ago

I'm honestly curious how my pack is going to function in 1.13. The main goal is that you can update from 1.12 to 1.13 and just keep going with the same world. It's rather uncharted territory, but it's at least in the realm of feasibility from my testing with moving between other packs.

lehjr commented 5 years ago

Honestly, I think 1.13 is going to be a mod breaking update, at least initially. One of the things I've seen is new modding API's like Rift and Fabric. I tried Rift and just deleted my workspace 15 minutes later. Fabric I can't comment on.

eyeonus commented 5 years ago

Yeah, I can see. If it weren't for the code being called every tick, it wouldn't be so bad, because you could just have each thing get the current modifier, do a manipulation on that, and set the modifier to the manipulated value. This would give the right speed on the first run through, but as soon as the code gets called on the second tick, everything goes batty.

I have an idea that might work , maybe even without being super messy:


Make an oldValue, initialized to 0, for each speed-affecting slider. (Maybe even as a list instead of a bunch of individual values.)

Whenever a slider value changes, including when the module the slider is a part of is (un)installed, the curValue will be different from the oldValue, right? So do this:

In the tick event, check 'if oldValue != newValue'

If true, get current modifier value, unapply oldValue, apply newValue, set modifier as result, set newValue as oldValue.

(ex:

newSpeed = getMovementModifier(item, walkMultiplier);
newSpeed /= 1 + oldBoost;
newSpeed *= 1 + newBoost;
setMovementModifier(item, newSpeed);

)

(If false, do nothing.)


Unless I'm missing something major, which is likely since I haven't done more than glance at a small bit of the code, that should work fine, and not interfere with any other mods that also mess with the speed.

(I am also assuming that all the speed modifier guys are multiplication/division, as well. If any are addition/subtraction, or exponentiation, or some other math, this probably wouldn't work.)

It's also entirely possible that that's not even possible to do, in which case sorry for wasting your time making you read this wall o'text. :)

ghost commented 5 years ago

I'm going to agree on that. I probably won't even begin to think about pack updating until a year after forge 1.13 moves to release. Then there will be all of the stragglers to prod to update or find replacements for.

Rift and fabric are sort of like liteloader, extensions of minecraft, but not an actual modding api. Rift even said it's not trying to compete with forge. I get where they're coming from, trying to fill a gap left by forge taking so long to update, but from what I've heard of forge 1.13 it will be worth the wait.

ghost commented 5 years ago

On the topic of kinetic generators, I want to see if I understand the config file correctly The base for the generator is the minimum, is the multiplier the maximum like it seems to be acting for me? In my pack I AM actually tweaking the config, and it seems to add the multiplier on top of the base for maximum. I doubt many people prod the config for this mod, but it might be worth rethinking the value name because for me looking at it I would assume the max would be the base TIMES the multiplier for max (which would make it REALLY OP).

lehjr commented 5 years ago

for the next release, the minimum default value will be 0.01, and the max will be 0.49. But yeah, the way it works is that the real max is actually the sum of the 2, so the naming isn't quite accurate Also, the config values aren't overwritten, so they have to be updated manually.

lehjr commented 5 years ago

Well, with the new code I managed to get the resistance so high that the player can't move at all.

eyeonus commented 5 years ago

That sounds like progress.

Mostly out of curiosity, is the idea I posted above even feasible, or was it a waste of time to type it?

ghost commented 5 years ago

I'll keep that in mind for tuning. I don't mind it, it just confused me a bit. I also found out that if you change the values of the batteries, you have to remove them from the suit then put new ones in to get the new values. I can get the new values by generating a new file and doing manual comparison, I don't mind, and I appreciate not having configs blown away with mod updates (I've had to deal with so much of that it's unreal, and it's very blatant because my pack is so heavily tuned).

and lol, that's not progress xD. There is no such thing as wasted typing of ideas, the only bad question is the one not asked.

eyeonus commented 5 years ago

I was attempting to use sarcasm in a humorous way. :P

ghost commented 5 years ago

I know :P

lehjr commented 5 years ago

Basically, the calculations will end up being done in the sprint assist's setMovementModifier method with a check to see if the kinetic generator module is active or not and apply the calculation if it is.

The problem with doing calculations based on old values is that it's impossible to figure out where that value came from, so using that could throw the calculations off exponentially.

eyeonus commented 5 years ago

Not if you're the one that set the old value....

lehjr commented 5 years ago

Doesn't matter who set the value, the attribute modifier isn't set directly by the slider and setting it from 2 different sources one with a boost value, and then with a "hinder value" (terminology? ) won't have any positive effects. Also keep in mind there is no guarantee of a load order for which module will tick first.

eyeonus commented 5 years ago

Oh, I see, there's some misunderstanding. The old value is the old value OF THE SLIDER, not the old value of the attribute. So, regardless of what the current modifier value is, we know that at some point the old slider value was applied to it. Even if something else then changed the modifier value as well, as long as any modification was applied as a multiplication/division, it doesn't matter when or how many different modifications were done.

We know that the old slider value was applied, so when the slider value is changed, we remove the old slider value which we stored as oldValue, then apply the new slider value and store it as the new oldValue.

Remember, ((a b) c) / b == ((a b) / b) c == a * c

lehjr commented 5 years ago

but remember, the value from 2 separate modules is being applied to one attribute modifier. On top of that is that the calculation from the sprint assist module depends on whether the player is running or not. Also, where are you storing values? It's not possible to store values in the modules as they're singleton objects.

lehjr commented 5 years ago

btw, on that sprint assist... the actual modifier being applied is 2.5 (at max), but I think that's 2.5 plus the original speed. The max value is 1024, not that anything like that would ever be able to be used.

eyeonus commented 5 years ago

The first point doesn't matter if both are using multiplication to apply themselves, because multiplication is atomic. It doesn't matter if the speed boost is applied before, after, or not at all in relation to the resistance, and vice-versa. As long as they are both multiplication, you'll get the same result.

Hard numbers:
speed = 1
resistance = 0.5
boost = 2

speed = speed * (1 - resistance) // == 0.5
oldResistance = resistance
speed = speed * boost // == 1
oldBoost = boost
resistance = 0.25
speed = speed / (1 - oldResistance) // == 2
speed = speed * (1 - resistance) // == 1.5
oldResistance = resistance

speed = speed * boost // == 2
oldBoost = boost
speed = speed * (1 - resistance) // == 1
oldResistance = resistance
resistance = 0.25
speed = speed / (1 - oldResistance) // == 2
speed = speed * (1 - resistance) // == 1.5
oldResistance = resistance

It doesn't matter in what order the two multipliers are applied

The sprint assist thing is a valid point, but I can get around that doing the same kind of thing. It would require knowing whether or not the player was sprinting the previous tick, which means a boolean:

on Tick {
// nothing needs done if player was and is, or was not and is not, sprinting
if wasSprinting != isSprinting {
    // wasn't sprinting, but is now
    if isSprinting { 
        speed = speed / walkBoost
        speed = speed * sprintBoost
    }
    //was sprinting, but isn't now
    else {
        speed = speed / sprintBoost
        speed = speed * walkBoost
    }
wasSprinting = isSprinting
}

As far as where to store the values, wherever you currently store all the other variables for ArmorPieceX? Or wherever it is you store the current value of the sliders? Really anywhere that persists works, as long as the module code can access and update the value.

lehjr commented 5 years ago

The problem is those "hard numbers" aren't stored. The values from the sliders are not the final values. Remember, the sliders only store a double in range from 0-1. There's a sort of cache that I added, but that cache is wiped any time the sliders change. Plus, I don't want to keep adding a bunch of nbt tag that aren't needed. The model stuff does that enough already.

eyeonus commented 5 years ago

See now you're just making up problems.

double sprintMultiplier = ModuleManager.computeModularProperty(item, SPRINT_SPEED_MULTIPLIER);

sprintMultiplier is the "final value", so you'll want to store an oldSprintMultipler, so you can see if the current sprintMultiplier and the oldSprintMultiplier are equal or not, and if not, divide out the oldSprintMultiplier, multiply in the sprintMultiplier, and save the sprintMultiplier as oldSprintMultiplier. Same with the other values.

As to "nbt tag that aren't needed", if nbt tags is how stuff needs to be stored in order to persist, then in order to do things this way, they are needed. And there's exactly four of them, so it's not "a bunch".

You'd need the following: `` double oldSprintMultiplier double oldWalkMultiplier boolean wasSprinting double oldResistanceMultiplier



It doesn't really matter how those values are stored, as long as they are persistent and can be updated by the module code. If the only way to achieve that is with NBT tags, then okay, you need four NBT tags to do this.