Flat modifiers - #3378
Conversation
SpellDigger
left a comment
SpellDigger
left a comment
There was a problem hiding this comment.
as a side note, the way modifiers are applied are out of order, if we are really going to do this you should find a way to put the modifiers on some order so flat bonuses get added before/after multiplicative bonuses
|
yeah though it technically does have an order |
it infact does not have an order, its just the way good_at works imitates an order becuase the effect is applied when mining not on the tool stats itself |
|
ye |
|
yea so im asking YOU to make sure theres some sort of order between flat and multiplicative property modifications |
|
ye i am doing that |
| for (tempModifiers.items) |mod| { | ||
| mod.changeProceduralItemParametersFlat(proceduralItem); | ||
| } | ||
| for (tempModifiers.items) |mod| { | ||
| mod.changeProceduralItemParametersMult(proceduralItem); | ||
| } | ||
| for (tempModifiers.items) |mod| { | ||
| mod.changeProceduralItemParameters(proceduralItem); | ||
| } |
There was a problem hiding this comment.
this is a very inefficient way to do it, change the implementation to only need a single for loop
| pub fn changeProceduralItemParametersFlat(proceduralItem: *ProceduralItem, data: Data) void { | ||
| proceduralItem.setProperty(.swingSpeed, proceduralItem.getProperty(.swingSpeed) - data.flatStrength); | ||
| } | ||
|
|
||
| pub fn changeProceduralItemParametersMult(proceduralItem: *ProceduralItem, data: Data) void { | ||
| proceduralItem.setProperty(.swingSpeed, proceduralItem.getProperty(.swingSpeed)*(1 - data.multStrength)); | ||
| } |
There was a problem hiding this comment.
this is unecessary bloat, change the implementation in such a way that only changeProceduralItemParameters is needed
There was a problem hiding this comment.
ngl scrap that, u prob could get away with 2 functions instead of 3 mostly to account for cases like single_use having changeProceduralItemParameters override the properties
Wunka
left a comment
Wunka
left a comment
There was a problem hiding this comment.
You are changing a lot here.
- please state an exact reason for this. Why should this be added?
- please only touch the powerfull modifier and not rename strength to "multStrength", to have the least amount of changes. (I would even only keep the powerfull changes for testing and not to merge, as its another question which modifers should get this)
- as with powerfull, the uranium change will only be for testing should not be merged at the end.
|
I think we should try to reduce the number of modifiers, the current system was made in a time where we didn't have to consider armor and accessories yet. Duplicating this logic 1 million times is not very fun. |
IntegratedQuantum
moved this from Low Priority
to WIP/not ready for review
in PRs to review
4 participants
Allows for items to have flat bonuses as well as percentage bonuses
Usefull for addons and creating even more intersting materials
Also changes uranium to have less mass damage and hardness damage in exchange for a damage boost on the modifier
