sow/2026-Q2 SAM#377
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99e497947a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…asshopper component implemented
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ee3815f6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| adjacencyCluster = new AdjacencyCluster(adjacencyCluster, true); | ||
|
|
||
| mechanicalSystems = adjacencyCluster.SplitSystemsByZones<VentilationSystem>(zoneCategoryName, addPrefix); |
There was a problem hiding this comment.
SAMAnalyticalModifyMechanicalSystemsByZone is wired to split only VentilationSystem objects, so heating and cooling systems that span multiple zones remain unsplit even though the component is named and exposed as modifying mechanical systems broadly. In projects where zone-category partitioning is expected for all system categories, downstream reports and assignments stay inconsistent for heating/cooling.
Useful? React with 👍 / 👎.
…difyVentilationSystemsByZone
Drop the Newtonsoft.Json package dependency from SAM.Core, SAM.Geometry, SAM.Math, SAM.Weather, SAM.Analytical, SAM.Architectural, SAM.Units and their Grasshopper/Rhino UI projects. The Newtonsoft types JObject, JArray, JToken, JValue, JProperty, JsonConvert, JsonSerializerSettings, Formatting, NullValueHandling and JTokenType are reintroduced in the new namespace SAM.Core.Json as a thin compatibility shim over System.Text.Json.Nodes. Source files swap their using directive from Newtonsoft.Json.Linq to SAM.Core.Json with no other changes. The shim's JTokenType getter, ToValue and ToNode are tightened to match the round-trip behaviour the SAM codebase relies on: - The Guid string-parse heuristic is removed because downstream switches (ParameterSet, RelationCluster, ParameterFilter) have no Guid case and would silently drop parameters whose value is a Guid string. - The Date string-parse heuristic is restricted to strings whose shape matches strict ISO 8601 date-time (YYYY-MM-DDTHH:MM:SS...), so names, labels and version tags no longer get reclassified as DateTime. - ToValue tries TryGetValue<string> before TryGetValue<Guid>/<DateTime> so JSON string values surface as strings unless explicitly typed. - ToNode emits doubles, floats and decimals with an explicit decimal point via JsonNode.Parse so that 5.0 survives the round trip as Float instead of being silently demoted to Integer by System.Text.Json's default trailing-zero trimming. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The three accessors return the result on lookup failure and the default sentinel on success — branches were inverted versus their sibling accessors (ToString, ToDouble, ToInt, ToBool, ToColor). Effect: any caller asking for a DateTime/JObject/JArray parameter always received DateTime.MinValue or null regardless of whether the value was present. Pre-existing bug, predates the System.Text.Json migration; surfaced by the new ParameterSet round-trip test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
xUnit project targeting net8.0 that exercises SAM JSON serialization through the public IJSAMObject surface only — no JObject type is named, so the same source runs unchanged against Newtonsoft and against the System.Text.Json shim. JSON equivalence is structural via System.Text.Json.JsonDocument (order-insensitive on object keys, order-sensitive on arrays, whitespace-independent). Coverage: - ParameterSetTests — one test per JTokenType branch in ParameterSet.FromJObject (String, Integer, fractional Double, whole-number Double, Boolean, DateTime, Guid, Color, mixed) plus a heuristic guard for strings that look like dates. - SAMObjectTests — Guid/Name preservation and two-pass idempotency. - LibraryFixtureTests — every SAM_*Library.JSON under files/resources/Analytical/ is loaded, deserialized, re-serialized and structurally compared. Current state: 26/28 pass. The remaining 2 failures (StringThatLooksLikeDate, InternalConditionLibrary non-idempotency on LightingOccupancyControls) are pre-existing on master and not introduced by the migration. Not added to SAM.sln; build and run from SAM/SAM.Tests directly with dotnet test until it's wired into the main solution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…t) (#20) GZipStream.Read is not guaranteed to fill the buffer in one call; for large payloads (e.g. a whole compressed model) it returned only the first chunk, leaving the rest of the buffer zero-filled and producing a truncated, unparseable string. Loop the read until dataLength bytes are read (or the stream ends). Surfaced by SAM_UI undo/redo snapshots: restoring a multi-MB model via Query.Compress/Decompress failed (returned null) so undo did nothing; small payloads happened to fit one Read and worked. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Panels with azimuth just below 360 (e.g. 359.98) were rounded to 360, which falls outside the wrap-split sector intervals (316→359 & 0→44), so they were silently skipped. Wrap the rounded value back since 0° ≡ 360°. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… count BuildIntervalRatioMap clamped the apertureConstructions index with ratios.Count-1 instead of apertureConstructions.Count-1, risking an out-of-range access when the lists differ in length. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
NormalizeAngleDegrees/ClampTo360 return [0, 360) and preserve fractions (e.g. 359.98), not [0, 359] as the summaries claimed. No behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
SAMAnalyticalFilterByGeometry Grasshopper component input updated
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
* feat(geometry): NTS-backed topological validity check and repair Adds two additive entry points to SAM.Geometry.Planar.Query to close the gap where SAM could neither detect nor proactively repair topologically invalid 2D geometry: - IsValidTopologically: delegates to NetTopologySuite's IsValidOp so self-intersections, mis-oriented rings and overlapping/escaping holes are detected (SAM's existing IsValid only checks null/NaN/point-count), with an out-parameter reason for diagnostics. Overloads for NTS Geometry, Face2D and Polygon2D. - Fix: repairs invalid geometry via NetTopologySuite's GeometryFixer (the modern, area-preserving replacement for the Buffer(0) trick), previously only invoked reactively inside Union/Difference/Intersection fallbacks. Overloads for NTS Geometry, a single Face2D (largest repaired face) and a Face2D collection that keeps every face a repair yields, so a self- intersecting bowtie splits into its two valid faces rather than being lost. Adds GeometryValidityTests covering valid pass-through, bowtie detection with reason, single-face repair and collection repair preserving all faces. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fbzqm96QTEdKaTPLDrH6pQ * fix(geometry): preserve unconvertible faces and guard NTS conversion Addresses Codex review on PR #23: - Fix no longer drops a valid Face2D whose edges have no NetTopologySuite representation (e.g. Circle2D/Ellipse2D, which implement IClosed2D but not ISegmentable2D so ToNTS returns null). Conversion now happens once and a null result is treated as pass-through: the face is returned/kept unchanged rather than reported invalid and silently dropped by the collection overload. - IsValidTopologically and Fix wrap the SAM->NTS conversion in try/catch so a structurally broken Face2D/Polygon2D (e.g. fewer than three points, or coordinates that collapse after rounding) that would throw while building an NTS LinearRing now reports false / is handled, instead of throwing. Adds Fix_CurvedFace_PreservedNotDropped covering the Circle2D pass-through. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fbzqm96QTEdKaTPLDrH6pQ --------- Co-authored-by: Claude <noreply@anthropic.com>
) * Complete automatic bidirectional GH <-> SAM geometry casting Extend GooSAMGeometry CastFrom/CastTo so native Grasshopper/Rhino geometry links directly to/from SAM geometry without explicit converter components. CastTo (SAM -> native) gaps filled: - Face3D and Surface now cast to Brep/GH_Brep and GH_Surface (previously only Shell was handled). - Segment3D casts to GH_Line/Line; Segment3D/Polygon3D/Polyline3D/Polycurve3D cast to GH_Curve and raw Rhino Curve (previously only a raw Polyline). - Face3D now meshes to GH_Mesh, and Shell/Face3D/Mesh3D cast to raw Rhino Mesh. CastFrom (native -> SAM) gaps and bug fixed: - Mesh/GH_Mesh branches set Value but never returned, letting a mesh fall through; now null-guarded and returning. - Added handling for Rhino Line/Curve (the underlying types of GH_Line/GH_Curve after goo unwrapping) plus defensive GH_Line/GH_Surface/GH_Brep branches. All conversions reuse existing Convert/Rhino.Convert helpers; no new conversion logic. Single file changed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLHhK1UntAV25az7q7ixW2 * Fix GH_Curve/GH_Line/GH_Surface CastTo to use IsAssignableFrom Addresses review comment: using exact == on goo types means casts fail when Grasshopper queries a generic Geometry parameter (which provides a base type like IGH_GeometricGoo rather than the exact derived type). Changed to IsAssignableFrom, consistent with the existing GH_Brep/GH_Mesh branches. Raw struct targets (Line, Curve) are correctly left as ==. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HLHhK1UntAV25az7q7ixW2 --------- Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
https://github.com/HoareLea/SAM/blob/ffeb6f9cfd0befa30f74edb41afe68b966ad1b0a/SAM.Analytical/Create/MechanicalSystem.cs#L22
Changing the public two-argument string overload from (MechanicalSystemType, string id) to this three-argument optional form removes the old (MechanicalSystemType, string) method from the assembly metadata. Any existing plugin or script compiled against Create.MechanicalSystem(type, id) will fail to bind at runtime (and source callers using a positional string now have to disambiguate), so keep a two-parameter overload that forwards the string as id and add the prefix overload separately.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| result.Add(new GH_SAMParam(gooAnalyticalObjectParam, ParamVisibility.Binding)); | ||
|
|
||
| result.Add(new GH_SAMParam(new global::Grasshopper.Kernel.Parameters.Param_Brep() { Name = "_brep", NickName = "_brep", Description = "Brep", Access = GH_ParamAccess.item, Optional = true }, ParamVisibility.Binding)); | ||
| result.Add(new GH_SAMParam(new global::Grasshopper.Kernel.Parameters.Param_Brep() { Name = "_breps", NickName = "_breps", Description = "Breps", Access = GH_ParamAccess.list, Optional = true }, ParamVisibility.Binding)); |
There was a problem hiding this comment.
Existing Grasshopper documents saved with this component GUID still have the geometry input named _brep, but the component now defines and reads only _breps. In those saved definitions Params.IndexOfInputParam("_breps") will not find the wired input, so the component returns the unfiltered analytical objects instead of applying the brep filter until users recreate the component or manually update the parameter name.
Useful? React with 👍 / 👎.
Co-authored-by: Jakub Ziolkowski <jakubziolkowski@digiproject.uk>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43061e5220
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| public static MechanicalSystem MechanicalSystem(MechanicalSystemType mechanicalSystemType, string id = null) | ||
| public static MechanicalSystem MechanicalSystem(MechanicalSystemType mechanicalSystemType, string namePrefix = null, string id = null) |
There was a problem hiding this comment.
This public overload used to be MechanicalSystem(MechanicalSystemType, string id = null). By inserting namePrefix before id, existing source such as Create.MechanicalSystem(type, "3") still compiles but now treats "3" as a name prefix and leaves Id null, so callers that create systems with explicit IDs get different names/FullName values. Add a separate two-argument overload or keep id as the second parameter to preserve the old call semantics.
Useful? React with 👍 / 👎.
…nd GooSAMGeometry (#25) Two bugs in the cast path: 1. GooSAMGeometry.CastTo<Y>: `typeof(Y) is ISAMGeometry` is always false because typeof(Y) returns a System.Type, not an ISAMGeometry instance. Fixed to `typeof(ISAMGeometry).IsAssignableFrom(typeof(Y))` with a Value is Y guard. 2. GooObject.CastTo<Y>: after wrapping the geometry in a GooSAMGeometry, the code called CastTo<Y> on it — but when Y is GooSAMGeometry or IGooSAMGeometry no inner check matches the wrapper itself, so it always returned false. Added `is Y` early-return before the CastTo<Y> call. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20eebd11a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jsonObject["X"] = coordinates[0]; | ||
| jsonObject["Y"] = coordinates[1]; |
There was a problem hiding this comment.
When a Point2D carries non-finite coordinates, such as Point2D.Invalid or a point copied from null, these assignments put NaN/Infinity directly into a JsonObject. With the new System.Text.Json path, saving/exporting later via ToJsonString() rejects those values, so models containing an invalid point can no longer be serialized; use the same non-finite guard/Query.ToJsonNode path used elsewhere before adding coordinates.
Useful? React with 👍 / 👎.
* fix(geometry): add Mesh3D Transform overloads + Extrusion CastFrom in GooSAMGeometry Mesh3D: no Transform(Mesh3D, Transform3D/Matrix4D) overloads existed; dynamic dispatch in SAMGeometryTransform.cs fell through to Point3D as the closest match and threw an invalid-arguments error. Added both overloads — Matrix4D variant transforms each vertex via GetPoints()/GetTriangleIndexes loop and reconstructs with original connectivity; Transform3D delegates to it. GooSAMGeometry.CastFrom: Rhino Extrusion (referenced as native Extrusion or GH_Extrusion from a Referenced Extrusion param) had no handler, causing "Data conversion failed from Extrusion to ISAMGeometry". Both types now convert via extrusion.ToBrep(cap: true) and reuse the existing Brep→SAM path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(grasshopper): address Codex cast correctness issues GooSAMGeometry.CastTo: the IGH_GeometricGoo assignability guard was backwards — typeof(Y).IsAssignableFrom(typeof(IGH_GeometricGoo)) is true only when Y is a supertype of IGH_GeometricGoo (e.g. object), so the ToGrasshopper() fallback never fired for concrete goo targets like GH_Curve or GH_Brep. Flipped to typeof(IGH_GeometricGoo).IsAssignableFrom(typeof(Y)). GooObject.CastTo: the gooSAMGeometryInstance is Y early-return was too broad — when Y=object it returned the GooSAMGeometry wrapper instead of the underlying SAM geometry, breaking generic/object Grasshopper inputs. Restricted to fire only when Y is an IGH_Goo target. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…aram (#27) Both methods threw NotImplementedException, crashing Grasshopper when the user right-clicked and chose 'Set ISAMGeometry/Set Multiple ISAMGeometries'. Implemented using GetObject with a broad geometry filter (Brep, Mesh, Curve, Point, Extrusion, Surface) so any Rhino geometry can be picked and converted to ISAMGeometry via the existing CastFrom path. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Convert.ToRhino(Mesh3D) rebuilt each triangle from its three edge lines via Mesh.CreateFromLines at a distance tolerance and silently skipped (continue) any triangle it could not reconstruct. On meshes with coincident/near-coincident or unwelded duplicate vertices (e.g. a Rhino mesh imported to SAM with 1594 vertices but only ~499 unique coords) this dropped faces, so baking or previewing the Mesh3D showed holes even though the SAM Mesh3D stored every triangle. Build the Rhino mesh directly from the mesh's shared vertices (GetPoints) and face indices (TrianglesCount / GetTriangleIndexes): every triangle becomes exactly one face, with no tolerance-dependent reconstruction and no dropped faces. This is the bake (GH_Mesh.ToGrasshopper) and viewport-preview (DrawViewportMeshes) path. Generated by Michal Dengusiak & Claude Code Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…es (HoareLea#53 Section E quick-win) (#29) * perf(serialize): drop redundant DeepClone of fresh ToJsonObject() nodes ToJsonObject() builds and returns a brand-new, parent-less JsonNode, so cloning its result before attaching it to the parent tree is pure waste - it doubled allocations recursively on every serialize. Removed at the five sites that did `X.ToJsonObject()...DeepClone()`: - RelationCluster.WriteRelationValue (hot: runs per relation value, i.e. every related object in an AnalyticalModel) - Tag (IJSAMObject value, and SAMColor value) - PolynomialModifier - WeatherYear (per WeatherDay) This is the HoareLea#53 Section E "quick win": cuts the undo-snapshot ToJsonObject() tree-build cost (~10s on the 10k-space model) without the larger streaming-serializer rewrite. All 110 SAM.Tests pass (round-trip serialize/deserialize unchanged), confirming the parent-less invariant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Guard arbitrary-IJSAMObject sites against non-fresh ToJsonObject() (Codex) At the two sites where the value is a statically-arbitrary IJSAMObject (Tag IJSAMObject value, RelationCluster.WriteRelationValue), the public IJSAMObject contract does not guarantee ToJsonObject() returns a fresh parent-less node. An implementation returning a cached/parented node would, without a clone, throw under the JsonNode single-parent invariant on the next attach. Keep the perf win in the common (fresh) case and restore safety: attach the returned node directly when it is parent-less, clone only when it already has a parent. The three concrete SAM-owned sites (SAMColor, PolynomialEquation, WeatherDay) build fresh nodes by construction, so they stay direct. All 110 SAM.Tests still pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be64830b02
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| jsonObject["X"] = coordinates[0]; | ||
| jsonObject["Y"] = coordinates[1]; | ||
| jsonObject["Z"] = coordinates[2]; |
There was a problem hiding this comment.
When a Point3D contains NaN/Infinity (the copy constructor explicitly fills NaN for a null source), these assignments put those non-finite doubles into a JsonObject. Under the new System.Text.Json serialization path, later ToJsonString()/file export rejects non-finite JSON numbers, so any model that carries an invalid 3D point can fail to save instead of round-tripping; serialize via the finite-safe node path or omit/null the coordinates.
Useful? React with 👍 / 👎.
* perf(Solver2D): cap iterations when ShiftDistance is non-positive In Solve(), each label's candidate offset is StartingDistance + i*ShiftDistance. When ShiftDistance <= 0 the offset does not grow with i, so every one of the IterationCount passes tests identical positions - repeating the sweep is pure cost. Run a single pass in that case (both the Point2D and Polyline2D paths), which guards any caller that passes a degenerate (zero/negative) ShiftDistance from an IterationCount-fold blow-up. Placement results are unchanged for the positive-ShiftDistance path used by normal callers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Solver2D: bound degenerate layouts with a consecutive-unplaced backstop The non-positive-ShiftDistance guard alone did not cover the real floor-plan pathology: a section taken at the wrong elevation collapses every space to a thin sliver (near-zero area but a *large* farthestPointDistance, so the shift stays positive). No label centre fits its sliver LimitArea, so each of ~1703 labels runs its full 100x8 sweep before failing - a ~2-minute solve. Add a geometry-agnostic backstop: count consecutive unplaced labels and, once a long run (32) signals a degenerate layout, give each remaining label a single anchor attempt instead of the full sweep. Any successful placement resets the counter, so a normal plan with the odd unplaceable label is unaffected. Keeps the existing non-positive-ShiftDistance early-out. All 110 SAM.Tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Solver2D: add hard wall-clock budget as a universal cap The consecutive-unplaced backstop only bounds the case where labels FAIL to place. A degenerate floor-plan layout can also be slow while every label SUCCEEDS - e.g. when all anchors collapse onto the same point, each label still places but only after spiralling out past a growing pile of already-placed rectangles (O(N^2)), which the failure counter never trips. Add a 5 s wall-clock budget to Solve(): once exceeded, remaining labels skip the search and fall back to their anchor. This bounds the whole solve regardless of the failure mechanism. A full 10k-label plan solves in ~0.1 s, so a normal solve never approaches the budget. All 110 SAM.Tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * Solver2D: over-budget labels anchor (kept visible) + raise cap to 10s Diagnostic on the pathological 10k floor-plan view showed the budget was firing correctly but the consumer blanks the text of any unplaced (null) label, so the budget made ~1473 tags vanish. Over-budget labels now fall back to their anchor rectangle (non-null) instead of null, so they stay visible (possibly overlapping) rather than dropping. Raised the wall-clock cap 5s -> 10s so more tags place on dense-but-valid views before the fallback kicks in. All 110 SAM.Tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…ea#58) (#31) Constant-factor speed-ups for the clustered-anchor case behind SAM_UI HoareLea#58, with no change to placement results (validated by all 110 SAM.Tests): - RectangleGrid.Query allocated a fresh HashSet on every call to de-duplicate the rectangles a query box's cells share. On a clustered floor-plan solve that is ~1M allocations (one per candidate per label). Reuse a single scratch set cleared per call - Query is enumerated fully and sequentially by the solver, so one shared set is safe. Same de-dup, same results, far less GC. - generateOffsets() (the 8 fixed search directions) was rebuilt for every label; build it once before the loop. These cut the constant factor; the underlying O(N^2) when anchors cluster (the core of HoareLea#58) still needs the algorithmic change tracked in that issue. Left as a PR for real-model (10k) profiling before merge. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3 participants
Summary
This is draft PR for this quarter
Validation
How to verify or test the change.
@codex review