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 wraitii)

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

attachment:reproduce.patch:ticket:3908.

Change History (19)

comment:1 by fatherbushido, 8 years ago

I had just seen that there is a TODO in :r17386

// TODO: Support parsing of data types other than fixed; log warnings in other cases

comment:2 by fatherbushido, 8 years ago

Milestone: BacklogAlpha 21
Priority: Nice to HaveShould Have

comment:3 by fatherbushido, 8 years ago

Description: modified (diff)

comment:4 by sanderd17, 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 elexis, 8 years ago

Description: modified (diff)

comment:6 by wraitii, 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 fatherbushido, 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 fatherbushido, 8 years ago

Attachment: cost.diff added

comment:8 by fatherbushido, 8 years ago

Owner: set to fatherbushido
Resolution: fixed
Status: newclosed

In 18500:

Use decimals instead of integers to allow relative templates for resource costs, as proposed by sanderd17, fixes #3818. Use relative templates following r17706.

comment:9 by fatherbushido, 8 years ago

Keywords: review removed

comment:10 by fatherbushido, 8 years ago

Cc: mimo added
Keywords: rfc added
Resolution: fixed
Status: closedreopened

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 fatherbushido, 8 years ago

Attachment: costroundinthesim.diff added

comment:11 by fatherbushido, 8 years ago

Keywords: review added; rfc removed

comment:12 by leper, 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 fatherbushido, 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 fatherbushido, 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:15 by fatherbushido, 8 years ago

In 18632:

Reverts r18500 (excepting the buildtime part of iberian and macedonian fortress templates) as #3818 must be fixed in a cleaner way. Fixes a typo. Refs #3818.

comment:16 by elexis, 8 years ago

Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:17 by wraitii, 7 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.