Opened 5 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 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.

Attachments (2)

cost.diff (2.7 KB ) - added by fatherbushido 5 years ago.
costroundinthesim.diff (4.7 KB ) - added by fatherbushido 5 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 by fatherbushido, 5 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, 5 years ago

Milestone: BacklogAlpha 21
Priority: Nice to HaveShould Have

comment:3 by fatherbushido, 5 years ago

Description: modified (diff)

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

Description: modified (diff)

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

Attachment: cost.diff added

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

Keywords: review removed

comment:10 by fatherbushido, 5 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, 5 years ago

Attachment: costroundinthesim.diff added

comment:11 by fatherbushido, 5 years ago

Keywords: review added; rfc removed

comment:12 by leper, 5 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, 5 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, 5 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, 5 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, 4 years ago

Milestone: Alpha 21Backlog

Backlogging due to lack of progress.

comment:17 by wraitii, 4 years ago

Description: modified (diff)

comment:18 by fatherbushido, 3 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. --- update Well I am still ineffectual at solving that problem. I always conclude it's needed to analyze the validation lib. The good point is that I learnt some stuff. ---

Last edited 3 years ago by fatherbushido (previous) (diff)

comment:19 by fatherbushido, 3 years ago

Owner: fatherbushido removed
Status: reopenednew

comment:20 by elexis, 2 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 Imarok, 2 years ago

Component: UI & SimulationSimulation

Move tickets to Simulation as UI & Simulation got some sub components.

Note: See TracTickets for help on using tickets.