Opened 8 years ago
Last modified 2 years ago
#3818 new defect
[PATCH] Number type problem with relative templates.
Reported by: | fatherbushido | Owned by: | |
---|---|---|---|
Priority: | Should Have | Milestone: | Backlog |
Component: | Simulation | Keywords: | patch |
Cc: | mimo | Patch: |
Description (last modified by )
When using relative template, for example
<Cost> <BuildTime>350</BuildTime> <Resources> <stone op="mul">0.8</stone> </Resources> </Cost>
in mace_fortess.xml,
We get an (expected) error due to wrong number type :
ERROR: RelaxNGValidator: Validation error: structures/mace_fortress:1: Type nonNegativeInteger doesn't allow value '799.9878'
see L157 of ps/trunk/source/simulation2/system/ParamNode.cpp
// TODO: Support parsing of data types other than fixed; log warnings in other cases
Attachments (2)
Change History (24)
comment:1 by , 8 years ago
comment:2 by , 8 years ago
Milestone: | Backlog → Alpha 21 |
---|---|
Priority: | Nice to Have → Should Have |
comment:3 by , 8 years ago
Description: | modified (diff) |
---|
comment:4 by , 8 years ago
Isn't this a bit normal? If the code expects an integer, giving 0.8 won't be valid. Note that 0.8 has no finite expansion in binary, so multiplication with a something that should theoretically give an integer might not always result in something integer after all.
If you want to have multiplication with a decimal, the cost itself should be allowed to be a decimal, and the code should handle decimals correctly (rounding in the simulation or in the gui).
comment:5 by , 8 years ago
Description: | modified (diff) |
---|
comment:6 by , 8 years ago
The problem here is mostly that we have been using integers for things that should probably be able to handle decimals (most stats). Integers should be reserved for attributes that really are only supposed to be integers and everything would go haywire (integers as enums - though I can't think of a use case for us), IDs, counts,...
This should therefore be fixed by switching things to handle decimals.
comment:7 by , 8 years ago
Keywords: | patch review added |
---|---|
Summary: | Number type problem with relative templates. → [PATCH] Number type problem with relative templates. |
I guess the idea of wraitii and sanderd17 is the right one. cost roundings are yet in the gui so it should be ok
by , 8 years ago
comment:9 by , 8 years ago
Keywords: | review removed |
---|
comment:10 by , 8 years ago
Cc: | added |
---|---|
Keywords: | rfc added |
Resolution: | fixed |
Status: | closed → reopened |
Following many discussions on irc, i hesitated to revert r18500. It seems an option can be to allow decimals in templates, but round in the sim. Here is a (not so) draft patch, the AI part must be checked carefully.
by , 8 years ago
Attachment: | costroundinthesim.diff added |
---|
comment:11 by , 8 years ago
Keywords: | review added; rfc removed |
---|
comment:12 by , 8 years ago
Keywords: | review removed |
---|
How about actually fixing that TODO and doing the rounding (or similar) when parsing the XML?
comment:13 by , 8 years ago
Thanks for your clever input. I'll revert r18500, and i (or another) will try to look at this TODO. It'll be cleaner.
comment:14 by , 8 years ago
I feel a bit incompetent to fix that TODO in a clean way.
The 2 (clumsy) solutions i saw are:
- distinguish the op like imul (for integers) and fmul (for fixed) for example.
- adding another attribute (like datatype = "int") in xml files.
comment:18 by , 7 years ago
That one is still a TOHAVE. I am now convinced that we should not try the workaround of doing an imul (in templates) but should use the schema instead as leper said many times.
comment:19 by , 6 years ago
Owner: | removed |
---|---|
Status: | reopened → new |
comment:20 by , 5 years ago
In r22003 by wraitii:
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
comment:21 by , 5 years ago
Component: | UI & Simulation → Simulation |
---|
Move tickets to Simulation
as UI & Simulation
got some sub components.
I had just seen that there is a TODO in :r17386