Improved AddNode placement (+ additional fixes) #820
Conversation
This file contains 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
- Position new nodes without sibling overlap. - X/Z size is determined relative to parent extents and actual available space in target region. - Maximal X/Z size is kept to 20% of parent according to previous implementation. - X/Z size of new nodes is squarified. - Z height is a fixed world-space configuration value. - If space does not suffice, placement is prevented and a notification is displayed.
Ignore inactive nodes when checking overlap.
Place ResizeNodeAction handles on top of the resized node. This is especially helpful for nodes with a hight greater than the original intended architecture nodes (which virtuially had no height at all). Previously, it was possible to take the cursor movement directly via a raycast on a parent node because the handles were assumed to be flat on the surface. This assumption is not true when the nodes have a miscellaneous height and/or when the handles are positioned on top. A new mechanism translates the 2D movement of the cursor into the 3D world space. However, this is not yet compatible with VR controls.
Use BitIncrement() and BitDecrement() in ResizeNodeAction for setting float values near thresholds while preventing redetection instead of using a static offset that might not work in specific edge cases.
The edge shader is modified to handle opaque vs. transparent fragments in separate passes. Opaque elements write to the depth buffer (ZWrite) so that the order is preserved. This fixes the problem of resize handles occluding edges that should in fact be drawn over the handles. Please note that other approaches were problematic: 1. Keep the shader as-is but enable ZWrite: This solves the issue described above but transparent parts would occlude labels using TextMeshPro shaders. 2. Using ZWrite and pushing depth buffer values for semi- transparent fragments back just before the clipping plane, e.g., 0.99: This does not work the same way as not writing to the depth buffer, thus has several side effects and still interferes with TMP shaders.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Allow passing server configuration via environment.
falko17
requested changes
Jan 17, 2025
| /// This is necessary to compensate for precision fluctuations in float values. | ||
| /// </para> | ||
| /// </summary> | ||
| private const float tolerance = 0.0001f; |
There was a problem hiding this comment.
Note that there exists a Mathf.Approximately function that we have been using so far for this purpose. Unless we need to specify the tolerance here as something different from Mathf.Epsilon, we should probably prefer that one.
| Bounds2D potentialGrow = new(0f, 0f, 0f, 0f); | ||
| foreach (Transform sibling in parent.transform) | ||
| { | ||
| if (!sibling.gameObject.IsNodeAndActiveSelf()) |
There was a problem hiding this comment.
This method is very long. Is it possible to factor some parts out of it? For example, could we extract the contents of each of these foreach loops into their own (possibly local) method?
| { | ||
| continue; | ||
| } | ||
|
|
|
|
||
| // Determine shrink direction: weight by area size | ||
| float area = 0f; | ||
| float potentialArea; |
There was a problem hiding this comment.
Why is this variable declared here? It seems that it's re-defined in every if block and not used outside of them.
| } | ||
|
|
||
| // Grow to fill available space | ||
| foreach (Direction2D direction in new Direction2D[] {Direction2D.Left, Direction2D.Right, Direction2D.Back, Direction2D.Front}) |
There was a problem hiding this comment.
We can leave out the type declaration here:
Suggested change
| foreach (Direction2D direction in new Direction2D[] {Direction2D.Left, Direction2D.Right, Direction2D.Back, Direction2D.Front}) | |
| foreach (Direction2D direction in new[] {Direction2D.Left, Direction2D.Right, Direction2D.Back, Direction2D.Front}) |
Comment on lines
+215
to
+220
| if (Front < other.Back || Back > other.Front || Left > other.Right || Right < other.Left) | ||
| { | ||
| // No overlap detected | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Can be rewritten as:
Suggested change
| if (Front < other.Back || Back > other.Front || Left > other.Right || Right < other.Left) | |
| { | |
| // No overlap detected | |
| return false; | |
| } | |
| return true; | |
| return Front >= other.Back && Back <= other.Front && Left <= other.Right && Right >= other.Left; |
| @@ -1033,18 +1060,18 @@ public static GraphElementOperator Operator(this GameObject gameObject) | |||
| } | |||
|
|
|||
| /// <summary> | |||
| /// Checks if <paramref name="gameObject"/> overlaps with any other direct child node of its parent. | |||
| /// Checks if <paramref name="gameObject"/> overlaps with any other acrtive direct child node of its parent. | |||
There was a problem hiding this comment.
Suggested change
| /// Checks if <paramref name="gameObject"/> overlaps with any other acrtive direct child node of its parent. | |
| /// Checks if <paramref name="gameObject"/> overlaps with any other active direct child node of its parent. |
| /// </summary> | ||
| public static partial class SEEMath | ||
| { | ||
|
|
| /// Please consult method descriptions for credits and license. | ||
| /// </para> | ||
| /// </summary> | ||
| public static partial class SEEMath |
There was a problem hiding this comment.
Why is this class partial?
| Please note that updating dependencies might require code changes. | ||
| It is usually safe -- and strongly suggested -- to bump patch level versions. | ||
| This is typically the third part of the version number, e.g., `1.2.3` -> `1.2.5`. | ||
| Changes to the first two segments are often associated to breaking changes and need additional manual review and testing. |
There was a problem hiding this comment.
This should only be true for the first segment, right? Or are the dependencies using something other than semantic versioning?
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.
This MR improves the placement of new nodes using
AddNodeActionand provides some additional fixes.AddNode placement
Previously, new nodes were placed anywhere with a fixed size of
0.2.This led to new nodes regularly overlapping with existing nodes and several other shortcomings.
The updated logic improves various key points:
MoveAction).0.2but is allowed to shrink based on the available space down to a defined minimal size.fixes #786
Miscellaneous changes
ZWritefor semitransparent parts off. This minimizes the display artifacts with the current scenes and does not interfere with TextMeshPro's shaders while fading edges out.OverlapsWithSiblings()now ignores inactive nodes (e.g., nodes that have been deleted but are actually still hanging around invisibly).BitIncrement()andBitDecrement()is used to get the next greater or next smaller float value.AddNodeAction,ResizeNodeAction).Here there are some screenshots visualizing the shader problem:
ZWrite on:Opaque edges for reference.
Semitransparent edges during fade-out occluding parts of the labels.