Skip to content

Two messaging fixes: Header.parse and the project envelope - #25

Open
gonzalocasas wants to merge 2 commits into
restructurefrom
pr/mqtt-and-model-fixes
Open

gonzalocasas wants to merge 2 commits into
restructurefrom
pr/mqtt-and-model-fixes

Conversation

@gonzalocasas

@gonzalocasas gonzalocasas commented Sep 7, 2026

Copy link
Copy Markdown
Member

Two independent bug fixes on top of the Model migration. Neither involves a design decision.

Header.parse scrambles every field

messages.py is identical on main and restructure, so this fix applies to both. It lives here, at the base of the messaging stack, rather than in a separate PR against main: the protobuf wire work in #26 stacks on top of it and touches the same file, and #23 is queued to merge, so main picks the fix up along with it.

Header.parse passed four values positionally into a constructor whose first parameter is increment_response_ID, shifting every field. Broken since 2024-05-01. Fixed with keyword arguments, the dropped validation restored, plus a zero-valued-counter bug (sequence_id or default discards a legitimate 0).

The project envelope drops elements for timber models

create_project_data_from_compas emitted beams/joints instead of elements for a TimberModel:

if isinstance(model, TimberModel):
    data = {..., "beams": ..., "joints": ...}   # no "elements"
else:
    data = {..., "elements": ...}

Consumers read model and elements — the C# runtime does exactly that — so a timber project silently produced a model with no elements at all.

The envelope is now defined in terms of a plain compas_model Model and always emits elements. Beams and joints stay available for timber-aware consumers, but as additive extras rather than a different contract. This follows the intent that COMPAS XR work with plain Model by default and treat compas_timber as a specialisation.

Fully removing the compas_timber dependency is a larger job and not attempted here — BuildingPlan, Step and SimpleSequenceGenerator run through the project API.

Also

Relaxes the compas_pb test pin from 1.1 to 1.x, matching requirements.txt (>=1.1,<2) and the 1.2 the C# runtime targets. The test asserted 1.1. specifically and would have failed on a legitimate, wire-compatible upgrade.

15/15 tests pass.

🤖 Generated with Claude Code

gonzalocasas and others added 2 commits September 8, 2026 01:39
Header.parse passed four values positionally into a constructor whose
first parameter is increment_response_ID, so every field shifted by one:
sequence_id landed in increment_response_ID, response_id in sequence_id,
device_id in response_id, time_stamp in device_id, and time_stamp was
regenerated. Verified over a live broker round-trip.

This has been wrong since 26577b9 (2024-05-01), which added
increment_response_ID as a new first parameter without updating the call
site. The version before it read each field by name.

Pass the fields as keyword arguments and restore the validation that
commit dropped. Two related fixes:

- The constructor used `sequence_id or default`, discarding a legitimate
  counter value of 0. Use `is None` instead.
- parse() now accepts either the decoded wire dict or a Header instance.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
create_project_data_from_compas emitted beams/joints instead of elements
for a TimberModel. Consumers read "model" and "elements" — the C# runtime
does exactly that — so a timber project silently produced a model with no
elements at all.

Define the envelope in terms of a plain compas_model Model and always
emit elements. Beams and joints stay available for timber-aware
consumers, but as additive extras rather than a different contract.

Also relaxes the compas_pb test pin from 1.1 to 1.x, matching
requirements.txt (>=1.1,<2) and the 1.2 the C# runtime targets.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@jckenny59 jckenny59 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for fixing my “creative interpretation” of Header.parse 😅

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.

2 participants