HomeWildfire Games

Add "mul_round" op to template parsing to support multiplying-then-rounding.
Concern RaisedrP22003

Description

Add "mul_round" op to template parsing to support multiplying-then-rounding.

This allows using arbitrary 'mul' values with Integer types, instead of having to switch them to Decimal types.
The ParamNode is not aware of validation (thus types) so a better solution is incredibly non-trivial.

Differential Revision: https://code.wildfiregames.com/D268

Event Timeline

elexis raised a concern with this commit.Jun 15 2019, 7:58 PM
elexis added a subscriber: elexis.
  • Integer/Schema:

Supporting an integer type in the schema sounds right regardless which templates 0ad provides.
I'm not so sure whether that wouldn't cover the feature better than mul_round.
The reason why someone would want to use mul_round in the template is that they want to specify an integer.

Going to rephrase this revision to address #3818.

That didn't happen, the ticket is open. The 'multiplied integer schema' use case is not implemented in this repository.

It seems there is no template in 0ad using this, so the use case is hypothetical.

I'm not sure if this changeset is useful if the ticket / integer multipliers is fixed, because those would address that use case.

To me it seems like a right patch to a wrong issue that is similar to a greater issue (validation support of integer types), especially since the intention was to fix the ticket at some point, but then on the other hand not fixing it and committing something that doesn't have a use case.

While it is ambiguous whether this operation is hypothetically useful, I error on the side of concern because it is an anti-pattern to commit things that add unused code that don't solve the stated goal in the correct way (correct as in solving that TODO to suffice the use case in a way that also extends to the schema).

/ps/trunk/source/simulation2/system/ComponentManager.cpp
1128
/ps/trunk/source/simulation2/system/ParamNode.cpp
174

This TODO from rP17386 was referenced by leper and fatherbushido in the revision proposal D268 and Trac:#3818.

This commit now has outstanding concerns.Jun 15 2019, 7:58 PM
/ps/trunk/source/simulation2/system/ParamNode.cpp
174

Doesn't actually relate to #3818 though.

  • Integer/Schema:

Supporting an integer type in the schema sounds right regardless which templates 0ad provides.

We support integers just fine in our XML schemas. The problem is that multiplying an integer by a decimal sometimes leads to non-integer values.

Going to rephrase this revision to address #3818.

That didn't happen, the ticket is open. The 'multiplied integer schema' use case is not implemented in this repository.

Apparently forgot to close it.

We support integers just fine in our XML schemas. The problem is that multiplying an integer by a decimal sometimes leads to non-integer values.

As far as I understood the idea is to be able to support op="mul" and op="add" with integer types and do the rounding if the type is an integer, otherwise don't do the rounding.
This way the schema / template can continue to use the integer type if that is the datatype intended by the template author.

We support integers just fine in our XML schemas. The problem is that multiplying an integer by a decimal sometimes leads to non-integer values.

As far as I understood the idea is to be able to support op="mul" and op="add" with integer types and do the rounding if the type is an integer, otherwise don't do the rounding.
This way the schema / template can continue to use the integer type if that is the datatype intended by the template author.

Mpf, that actually could be it. I thought it meant that the op should specify we want integer operations - bit unclear. Leper seemed to favour specific operators in https://code.wildfiregames.com/D268.
I'm not sure that silently handling that for the user is better than explicit handling - at least code-wise this is probably the less intrusive implementation.

I also don't care that much about this topic - apparently no-one did either since D268 never got much attention except from leper and the followup neither.

I also don't care that much about this topic - apparently no-one did either since D268 never got much attention except from leper and the followup neither.

  • You do care about that topic because you wrote a patch for it and proposed to close the ticket as fixed.
  • Noone cared about this since ever, but this isn't a sign that this isn't problematic, but rather uninvestigated.
  • If it receives attention of the people who worked on the code, then this is a reason to take that seriously.
  • The argument isn't that the patch is introducing a bad trait (other than arguably unused hypothetic code) but that the solution is not the better one to fix the task at hand unless there is further evidence for the this use case not being covered by the integer schema use case.

I also don't care that much about this topic - apparently no-one did either since D268 never got much attention except from leper and the followup neither.

  • You do care about that topic because you wrote a patch for it and proposed to close the ticket as fixed.

I don't care that much.

  • The argument isn't that the patch is introducing a bad trait (other than arguably unused hypothetic code) but that the solution is not the better one to fix the task at hand unless there is further evidence for the this use case not being covered by the integer schema use case.

This code works just fine, it's easy to maintain, it's readable. It's not certain that the integer-schema thingy would result in cleaner, more maintainable code - I didn't find a way to write it.
I don't really see the point of raising a concern for code that's perfectly fine, only it might not be the best possible implementation. You're just adding noise to the audit queue.