Fixing the throttle default values from 4zK's prior work #337
Conversation
Added flag for whether or not either throttle was redefined and, if it wasn't, defaulted it to 0 for AHumans' and ACrabs' jetpacks. Added setter for throttle values, and renamed the getters to be more consistent (though the throttle variables and getters and setters all need to be renamed at some point, there's an issue in for that) Added lua bindings for min and max throttle range. They've got garbage names because I don't want them bound til we get a better name for this stuff
# Conflicts: # CHANGELOG.md - Solved manually
# Conflicts: # CHANGELOG.md - Solved manually # Managers/LuaMan.cpp - Used theirs, will followup with entity bindings
…e/PositiveThrottleMultiplier`, Added corresponding lua bindings, Removed "ThrottleRangeRedefined" tag for being useless
| throttleFactor += std::abs(m_MaxThrottleRange) * m_Throttle; | ||
| float absThrottle = std::abs(m_Throttle); | ||
| if (m_Throttle < 0) { | ||
| throttleFactor = throttleFactor * (1 - absThrottle) + (m_NegativeThrottleMultiplier * absThrottle); |
There was a problem hiding this comment.
I'm pretty sure you've made this line more complicated than it needs to be. throttleFactor is set to 1.0F right above, but you're using it in your math so you effectively have 1.0F * (1 - std::abs(m_Throttle) + (m_WhateverMultiplier * std::abs(m_Throttle))
Also, can you explain the math changes here a bit, cause I'm pretty sure they're wrong.
Before throttle and throttle multipliers both linearly affected the resulting throttle factor (i.e. emission rate) in a totally straightforward way. Bigger throttle and bigger multiplier meant more emission rate, going from 0 (no emissions at min throttle) to 2 (double emissions at max throttle). Yours doesn't abs the multipliers, meaning that their signs matter (though reasonably they should always be 0 - 1 so that's not a big deal, but probably needs enforcement in the setters), and when I plug the numbers into excel your stuff is super weird (unless I've made a mistake):
If you wanna do it yourself, the formulas are:
Old - =1 + (A2*ABS(B2))
New - = 1 * (1-ABS(A2)) + (B2*ABS(A2))
There was a problem hiding this comment.
I gave the formulas to Trez in beforehand to be automatically handled by the converter. I guess it's a thing worth putting into the changelog entry even though it'll probably end up confusing modders, especially when this stuff hasn't ever been very apparent in mods.
I thought we discussed the throttle logic change already so I don't really know what you're frothing about.
There was a problem hiding this comment.
lol it's 100% possible that we did, but also possible that I've forgotten since it was a while back, or that I didn't fully understand.
Still, this seems straight up incorrect to me, like why would having throttle 0.1 and positive throttle multiplier 0.1 result in 0.9 strength emissions? Feel free to shoot me a discord message if you'd rather discuss there.
There was a problem hiding this comment.
After discussion with 4zK, we're keeping this as-is. There's a followup issue to get rid of these confusing code concepts - #374.
Entities/AEmitter.cpp
Outdated
| m_EmitCountLimit = 0; | ||
| m_MinThrottleRange = 1.0F; | ||
| m_MaxThrottleRange = 1.0F; | ||
| m_ThrottleRangeRedefined = false; |
There was a problem hiding this comment.
I think you've forgotten the point of this branch. Your changes before to humans and crabs made it so jetpacks would lose strength if they were running out of fuel, which we didn't want happening to mods by default, unless modders defined throttle multipliers.
The "useless" flag you deleted was what kept this from happening, cause there was a check in AHuman and ACrab create that says "if jetpack throttle ranges haven't been defined, default them to 0 to avoid compatibility problems".
Also, not to be a dick but you 100% didn't run this code to see if it works :ju:. I know cause you forgot to remove this aforementioned check (or update comments that say if throttle range values have been defined) or fix stuff using old method names that you changed.
There was a problem hiding this comment.
The default value for both of the new throttle multipliers is 1, which means that throttle will have no effect on emission rate. This means that jetpacks will function as before, unless the property is changed.
I will however take the blame for any other thing I've missed cus this had to be done by most inconvenient shuffling of branches
There was a problem hiding this comment.
Okay, I'll wait til you clean up then. I know the branch shuffling stuff was a pain, so thanks for dealing with it.
There was a problem hiding this comment.
The math of throttleFactor stuff deals with this in a slightly roundabout way, so it's fine as-is.
|
I can't officially approve since I made this PR, but I approve! |
Unbroke all the mods :P
Added flag for whether or not either throttle was redefined and, if it wasn't, defaulted it to 0 for AHumans' and ACrabs' jetpacks.
Added setter for throttle values, and renamed the getters to be more consistent (though the throttle variables and getters and setters all need to be renamed at some point, there's an issue in for that)
Added lua bindings for min and max throttle range. They've got garbage names because I don't want them bound til we get a better name for this stuff
Note, the issue for renaming throttle stuff is #321