Skip to content

Deprecate the point-valued range property in favour of range(point_xy(...), point_xy(...)) #1165

Description

@MaartenHilferink

Spun off from #1163, where the worked examples originally used Range = "[{-1,-1}, {2,2})" to define a 3x3 potential/proximity kernel and were rewritten to range(point_xy(-1s,-1s), point_xy(2s,2s)).

Problem

For point-valued units the range property takes its bounds as {a, b} string literals, in which the coordinate order is not visible in the text:

unit<spoint> k3x3: Range = "[{-1, -1}, {2, 2})";      // which of the two is the row?

The order is in fact fixed, not ambiguous: operator >> for Point<T> in rtc/dll/src/ser/PointStream.h:50 reads is >> "{" >> p.Row() >> ", " >> p.Col() >> "}", so {a, b} is {Row, Col}. Verified empirically with a deliberately non-square kernel - Range = "[{0,-1}, {1,2})" yields a 1-row x 3-col window that grows a seed cell horizontally.

But nothing in the config text says so, which is exactly the class of syntax that was banned elsewhere. The existing ban does not reach this property: dmsPoint_SetFirstCfgValue / cfg2dms_order_inplace in rtc/dll/src/geo/PointOrder.h:168-198 throwDmsErrD with "obsolete syntax for point data used. Use the point_yx operation to unambiguously define points or yx(numeric, numeric) syntax in data blocks.", but that path covers point data blocks. The range property string is read straight through the Point stream operator and never touches it, so it still parses silently and without any warning.

Proposal

Deprecate the range / cat_range property for point-valued units only, in favour of the explicit expression form:

unit<spoint> k3x3 := range(point_xy(-1s, -1s), point_xy(2s,  2s));

Confirmed to produce an identical unit (the #1163 verification configs pass unchanged after the rewrite).

Scope note: scalar ranges such as unit<uint32> u: range = "[0, 100)" have no coordinate-order concern and should keep working untouched. RangeProp in rtc/dll/src/tic/UnitClassReg.h:27-41 is a single template instantiated for every unit value type, so the deprecation needs to be conditional on the value type being a Point<T>, not applied to the property as a whole. cat_range shares the same template.

Blocker: SetDepreciated() on a property does not warn

Marking the PropDef deprecated is currently not enough. AbstrPropDef::IsDepreciated() is consulted in exactly one place - rtc/dll/src/tic/Xml/XmlTreeOut.cpp:1008 - where it merely hides the property from the detail page. No warning is emitted anywhere when a config sets a deprecated property. (The only current user of the flag is Expr vs CalcRule, rtc/dll/src/tic/TreeItemProps.cpp:210.)

Compare the operator-group machinery, which has a real two-stage path:

  • oper_policy::depreciated -> reportF(SeverityTypeID::ST_Warning, "depreciated operator {} used: {}."), rtc/dll/src/tic/MoreDataControllers.cpp:139
  • oper_policy::obsolete -> throwErrorF(...), rtc/dll/src/tic/MoreDataControllers.cpp:143

So this issue needs a property-level equivalent first. The natural choke point for .dms configs is ConfigProd::DoAnyProp (stx/dll/src/ConfigProd.cpp:665-676), which resolves the PropDef by name and calls SetValueAsCharRange - a warning there would cover every property assignment in one place. AbstrPropDef::SetValueAsCharRange itself would also catch the XML path (rtc/dll/src/tic/Xml/XmlTreeParser.cpp:144).

Suggested steps

  1. Add a warning path for deprecated properties (ConfigProd::DoAnyProp and/or AbstrPropDef::SetValueAsCharRange), mirroring the operator-group warn/error split.
  2. Flag range/cat_range as deprecated only for Point<T>-valued units, with a message naming the replacement: "use range(point_xy(...), point_xy(...)) to define point ranges unambiguously".
  3. Migrate the configs, then consider promoting the warning to an error in a later major version.

Migration impact

In the regression tree (tst): 19 occurrences across 7 files use the point-brace form, most of them potential kernels.

  • Operator/cfg/issue_1460.dms, Operator/cfg/OperatorDebug.dms (5x), Operator/cfg/SubItemConnect.dms, Operator/cfg/getprojectionbase.dms, Operator/cfg/issue_1372.dms, MapView/issue_219.dms
  • Projects/lus_demo_2023/cfg/demo/geography.dms - 9 kernel units (pot100Range ... pot02Range), the bulk of the work

A further 12 range = "..." uses in that tree are scalar and must stay as they are. The GeoDMS repo itself contains no point-brace range properties, so the change is confined to the regression configs and to user projects.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions