Closed 12345ieee closed 6 years ago
Looks good to me. What are your concerns about this implementation? You say you're not totally convinced this is the right approach, so you must have some concerns.
The implementation is fine, I'm just not sure that this is the best way to give control.
The proposal of having +x
-> 10
and -x
-> x
looked even worse to me.
Well, given what we're doing here, some ugliness is to be expected.
A more elegant way to do this would be to introduce a new field to the JSON and ignore the current field (which is ignored by the base game anyway). If the new field is missing, you would fall back to the hardcoded value of 40. This would sidestep any backwards compatibility issues with the unpatched game. To implement it, we would first have to add the ability to inject custom attributes, at least for injected fields, so you could annotate the new field of the JSON mapper class properly.
(I'm assuming that the JSON mapper ignores any fields it doesn't know about. If it fails with unexpected fields, this alternative approach won't work.)
Opened mostly to discuss about the patch, which doesn't convince me 100%, but seems the saner way to give control over the output number.
Also includes an extra check for decoy methods and some attribute cleanup.