new files and modified files - #990
Open
zama9 wants to merge 11 commits into
Open
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Adds incremental rectangle and circle packing layouts with historical position preservation and registers them for city rendering.
Changes:
- Implements incremental rectangle and circle packing algorithms.
- Extends packing models with hierarchy and history metadata.
- Adds new layout kinds and renderer factories.
Reviewed changes
Copilot reviewed 11 out of 17 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
Assets/SEE/Layout/NodeTransform.cs |
Associates transforms with packed nodes. |
Assets/SEE/Layout/NodeLayouts/RectanglePacking/PTree.cs |
Adds incremental rectangle operations. |
Assets/SEE/Layout/NodeLayouts/RectanglePacking/PNode.cs |
Adds hierarchy and rectangle metadata. |
Assets/SEE/Layout/NodeLayouts/IncrementalRectanglePackingLayout.cs |
Implements incremental rectangle packing. |
Assets/SEE/Layout/NodeLayouts/IncrementalRectanglePackingLayout.cs.meta |
Adds Unity metadata. |
Assets/SEE/Layout/NodeLayouts/IncrementalCirclePackingNodeLayout.cs |
Implements incremental circle layout integration. |
Assets/SEE/Layout/NodeLayouts/IncrementalCirclePackingNodeLayout.cs.meta |
Adds Unity metadata. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/TheCircle.cs |
Defines incremental circle state. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/TheCircle.cs.meta |
Adds Unity metadata. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/SpatialHashGrid.cs |
Adds spatial collision lookup. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/SpatialHashGrid.cs.meta |
Adds Unity metadata. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/IncrementalCirclePackerExtended.cs |
Adds PBD-based circle relaxation. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/IncrementalCirclePackerExtended.cs.meta |
Adds Unity metadata. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/IncrementalCirclePacker.cs |
Adds history-aware circle placement. |
Assets/SEE/Layout/NodeLayouts/CirclePacking/IncrementalCirclePacker.cs.meta |
Adds Unity metadata. |
Assets/SEE/Game/CityRendering/GraphRenderer.cs |
Instantiates the new layouts. |
Assets/SEE/Game/City/NodeLayoutKind.cs |
Registers new layout kinds. |
Files not reviewed (6)
- Assets/SEE/Layout/NodeLayouts/CirclePacking/IncrementalCirclePacker.cs.meta: Generated file
- Assets/SEE/Layout/NodeLayouts/CirclePacking/IncrementalCirclePackerExtended.cs.meta: Generated file
- Assets/SEE/Layout/NodeLayouts/CirclePacking/SpatialHashGrid.cs.meta: Generated file
- Assets/SEE/Layout/NodeLayouts/CirclePacking/TheCircle.cs.meta: Generated file
- Assets/SEE/Layout/NodeLayouts/IncrementalCirclePackingNodeLayout.cs.meta: Generated file
- Assets/SEE/Layout/NodeLayouts/IncrementalRectanglePackingLayout.cs.meta: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+74
to
+95
| foreach (TheCircle c in dealingCircles) | ||
| { | ||
| (string, float, Vector2) tupple = bufferLastPos.FirstOrDefault(l => l.Item1 == c.ID); | ||
| if (c.Radius < c.nextRadius) | ||
| { | ||
| ExpandFromCircleA(circles, c, c.nextRadius); | ||
| } | ||
| else if (c.Radius > c.nextRadius) | ||
| { | ||
| c.Radius = c.nextRadius; | ||
| } | ||
|
|
||
| List<TheCircle> notPlacedCircles = circles.Where(c => !c.IsPlaced).ToList(); | ||
|
|
||
| newNodeIDsSizes = notPlacedCircles.Select(n => (n.ID, n.Radius)).ToList(); | ||
|
|
||
| PackingCircles(circles, containerCenter, out containerRadius, newNodeIDsSizes); | ||
|
|
||
| List<(string, float, Vector2)> placedCircles = circles.Select(c => (c.ID, c.Radius, c.Center)).ToList(); | ||
|
|
||
| lastPositions[parent] = placedCircles; | ||
| } |
Comment on lines
+322
to
+325
| if (R > largestRadius) | ||
| { | ||
| largestRadius = R; | ||
| best = new TheCircle( |
Collaborator
Author
There was a problem hiding this comment.
it produces the largest radius which encloses the set of circles as close as possible this is what it meant to do
Comment on lines
+394
to
+398
| if (R > largestRadius) | ||
| { | ||
| largestRadius = R; | ||
| best = new TheCircle( | ||
| null, |
koschke
requested changes
Sep 1, 2026
koschke
left a comment
koschke
left a comment
Collaborator
There was a problem hiding this comment.
I read the code only superficially looking for coding style violations only. Not for semantic problems.
Please fix those.
| /// <summary> | ||
| /// Finds an empty position for the given circle that does not overlap with already placed circles. | ||
| /// </summary> | ||
| /// <param name="placedCircles"></param> |
| /// <summary> | ||
| /// Checks if the given position and radius overlaps with any of the already placed circles. | ||
| /// </summary> | ||
| /// <param name="pos"></param> |
| /// Computes the smallest circle that can surround all the given circles, | ||
| /// returning a new circle with the computed center and radius. | ||
| /// </summary> | ||
| /// <param name="circles"></param> |
| /// Computes the smallest circle that can surround all the given circles, | ||
| /// and resets the positions of all circles to be relative to the surrounding circle. | ||
| /// </summary> | ||
| /// <param name="circles"></param> |
| internal TheCircle ComputeSurroundingCircleAndResetCircles(List<TheCircle> circles) | ||
| { | ||
| if (circles.Count == 0) { return new TheCircle(null, Vector2.zero, 0); } | ||
|
|
This was referenced Sep 2, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| public class IncrementalCirclePacker | ||
| { | ||
| /// <summary> | ||
| /// Packs the given list of circles into a container, adjusting |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// Packs the given list of circles into a container, adjusting | |
| /// Packs the given list of circles into a container, adjusting |
| } | ||
|
|
||
| /// <summary> | ||
| /// Performs the packing of circles based on their last known positions, |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// Performs the packing of circles based on their last known positions, | |
| /// Performs the packing of circles based on their last known positions, |
| } | ||
|
|
||
| /// <summary> | ||
| /// Packs the given list of circles into a container, |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// Packs the given list of circles into a container, | |
| /// Packs the given list of circles into a container, |
| } | ||
|
|
||
| /// <summary> | ||
| /// Computes the smallest circle that can surround all the given circles, |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// Computes the smallest circle that can surround all the given circles, | |
| /// Computes the smallest circle that can surround all the given circles, |
|
|
||
| /// <summary> | ||
| /// Implements a mass-weighted centripetal relaxation algorithm to pack circles densely in a 2D space. | ||
| /// Utilizing Position-Based Dynamics (PBD), this class creates a tightly nested, center-seeking cluster |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// Utilizing Position-Based Dynamics (PBD), this class creates a tightly nested, center-seeking cluster | |
| /// Utilizing Position-Based Dynamics (PBD), this class creates a tightly nested, center-seeking cluster |
| /// This layout packs rectangles closely together as a set of nested packed rectangles to decrease | ||
| /// the total area of city. It also ensures that the layout is incremental, meaning that if a node | ||
| /// is added or removed, the layout will adjust accordingly without having to recompute the entire | ||
| /// layout from scratch. The algorithm finds the best position for each rectangle based on its size |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// layout from scratch. The algorithm finds the best position for each rectangle based on its size | |
| /// layout from scratch. The algorithm finds the best position for each rectangle based on its size |
| } | ||
|
|
||
| /// <summary> | ||
| /// A reference to the layout calculated in the previous frame or state. |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// A reference to the layout calculated in the previous frame or state. | |
| /// A reference to the layout calculated in the previous frame or state. |
| /// <summary> | ||
| /// A reference to the layout calculated in the previous frame or state. | ||
| /// This is strictly required for the "incremental" aspect of the layout, as the algorithm | ||
| /// uses the positions from this old layout to try and keep nodes as close to their previous |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// uses the positions from this old layout to try and keep nodes as close to their previous | |
| /// uses the positions from this old layout to try and keep nodes as close to their previous |
| public IncrementalRectanglePackingLayout oldLayout; | ||
|
|
||
| /// <summary> | ||
| /// Implements the IIncrementalNodeLayout interface property. Provides a safe setter to |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// Implements the IIncrementalNodeLayout interface property. Provides a safe setter to | |
| /// Implements the IIncrementalNodeLayout interface property. Provides a safe setter to |
|
|
||
| /// <summary> | ||
| /// Implements the IIncrementalNodeLayout interface property. Provides a safe setter to | ||
| /// inject the previous layout instance. It ensures type safety by throwing an ArgumentException |
Contributor
There was a problem hiding this comment.
Warning
Trailing whitespace detected! Please remove it.
Suggested change
| /// inject the previous layout instance. It ensures type safety by throwing an ArgumentException | |
| /// inject the previous layout instance. It ensures type safety by throwing an ArgumentException |
koschke
requested changes
Sep 3, 2026
koschke
left a comment
koschke
left a comment
Collaborator
There was a problem hiding this comment.
There are still many open issues left raised by Copilot and me. There are new issues with trailing whitespaces.
Please fix those.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds incremental rectangle and circle packing layouts with historical position preservation and registers them for city rendering.
Changes: