Skip to content

fix(xml): handle rational types in ImageSpec roundtrip - #5581

Open
ayushactiveat wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
ayushactiveat:fix/xml-rational-roundtrip
Open

ayushactiveat wants to merge 1 commit into
AcademySoftwareFoundation:mainfrom
ayushactiveat:fix/xml-rational-roundtrip

Conversation

@ayushactiveat

Copy link
Copy Markdown

Fixes #5571.

Description

Fixes two issues preventing rational types (like FramesPerSecond) from surviving an XML round-trip:

  1. String parsing: TypeDesc::fromstring() didn't recognize rational2i or rational2f. Added these mappings to typedesc.cpp.
  2. Value separator: The XML writer outputs 30000/1001 (with a slash), but the parser expected commas. Added logic in paramlist.cpp to handle the slash.

Tests

Added tests in typedesc_test.cpp for string parsing, and imagespec_test.cpp for the full XML round-trip. Both now verify that the type and the float value (29.97) survive to_xml() and from_xml().

Assisted-by: ByteAsk / GPT-5.4

Checklist:

  • I have read the guidelines on contributions and code review procedures.
  • I have read the Policy on AI Coding Assistants and added the Assisted-by line above.
  • I have updated the documentation if my PR adds features or changes behavior. (N/A)
  • I am sure that this PR's changes are tested in the testsuite.
  • I have run and passed the testsuite in CI before submitting the PR. (Local build failed due to missing dependencies, relying on GitHub CI).
  • My code follows the prevailing code style of this project.
  • If I added or modified a public C++ API call, I have also amended the corresponding Python bindings. (N/A)

@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: ayushactiveat / name: Ayush Singh (3cf27a4)

@ayushactiveat
ayushactiveat force-pushed the fix/xml-rational-roundtrip branch from 3cf27a4 to 8ca7943 Compare October 18, 2026 20:29
@lgritz

lgritz commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

OK, I understand the fix about how rationals were written to XML as "N / D" when they should have been "N, D".

But what's rational2f about and why is it needed?

@ayushactiveat

Copy link
Copy Markdown
Author

hi @lgritz , added rational2f because TypeDesc::c_str() can give that string for float based rationals, so just wanted the parser to handle everything the writer produces. if you like to keep focus only on the original issue for now, i can remove it, let me know

Fixes AcademySoftwareFoundation#5571. 1) TypeDesc::fromstring() now recognizes 'rational2i' and 'rational2f'. 2) ParamValue parsing now replaces the '/' in rational XML values with ',' so the numbers load correctly. Added tests for both.

Signed-off-by: Ayush Singh <ayushactiveat@gmail.com>
@ayushactiveat
ayushactiveat force-pushed the fix/xml-rational-roundtrip branch from 8ca7943 to c5c710b Compare October 19, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: ImageSpec XML roundtrip doesn't handle rationals

2 participants