APN-Pucky / tyrant_optimize

MIT License
15 stars 7 forks source link

Passive BGE Devotion: parameterized (as percent) #78

Closed dsuchka closed 2 months ago

dsuchka commented 2 months ago

Use "Devotion 20" to have 20% bonus, "Devotion 25" for 25% and so on

APN-Pucky commented 2 months ago

LGTM, two minor things I noticed:

https://github.com/APN-Pucky/tyrant_optimize/blob/ec83f466f15ee959e0e70ce77792474ff1636333/tests/test_crash.csv#L19

iirc was designed to catch crashes, maybe it should have Devotion 20 and/or Devotion 25. Now Devotion w/o percentage gives just 1%:

https://github.com/APN-Pucky/tyrant_optimize/blob/ec83f466f15ee959e0e70ce77792474ff1636333/tyrant_optimize.cpp#L2396

That is no problem to me, but something I wanted to raise here (I rarely use tuo nowadays).

Maybe you could add a X or N% to the GUI here after Devotion (no need to recompile just for the future)? https://github.com/APN-Pucky/tyrant_optimize/blob/ec83f466f15ee959e0e70ce77792474ff1636333/SimpleTUOLiveSim.ahk#L18

dsuchka commented 2 months ago

Now Devotion w/o percentage gives just 1%

Yes, I know about the issue. This is how the Passive BGE is implemented in the TUO. For some cases default value=1 is good choise (as for denominator for instance), for other ones — not.

I would keep it simple as possible and don't complicate the bge parsing process (to introduce default values), on the other hand I would also avoid workaround to use a default (20% or 40%) when it's 1%: no any guarantees that the Devotion BGE has a default (Devs use a new value every time as far as I see) and any default value is not correct ideologically (it's easy to forget to set a correct value next time). I would say it's better to throw an Exception when it's 1% rather than run a default value.

APN-Pucky commented 2 months ago

Thanks! I'll prepare a release.