Opened 8 years ago
Last modified 2 years ago
#3818 new defect
[PATCH] Number type problem with relative templates. — at Version 17
Reported by: | fatherbushido | Owned by: | fatherbushido |
---|---|---|---|
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
Change History (19)
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.
I had just seen that there is a TODO in :r17386