support multi-level walls #103

jamesbradleym · 2024-07-25T13:36:11Z

BACKGROUND:

Multi-level spaces do not create walls per level, instead the height of the wall is the height of those levels

DESCRIPTION:

Splits the walls from pringle by level prior to merging

Addresses: https://github.com/hypar-io/pringle/issues/1290

This change is

wynged

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 5 unresolved discussions (waiting on @jamesbradleym)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 1 at r1 (raw file):

using DotLiquid.Tags;

what's this for?

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 31 at r1 (raw file):

                }

                levels.Sort((level1, level2) => level1.Elevation.CompareTo(level2.Elevation));

doesn't matter much, but I might make the method below create the ordered list of levels internally rather than requiring that the levels being passed in be sorted. might prevent a mistake at some future date if we mis-order the levels or order them in some other way for some other reason, the SplitWallsByLevels method could just keep working if it did it all internally.

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 60 at r1 (raw file):

        }

        public static List<StandardWall> SplitWallsByLevels(List<StandardWall> walls, List<Level> levels, Random random)

making the walls argument be for IEnumerable<StandardWall> would make you not need to do ToList above.

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

                            var newWall = new StandardWall(
                                wall.CenterLine,
                                0.1,

what is 0.1?

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 103 at r1 (raw file):

                    else
                    {
                        newWalls.Add(wall);

can we reduce the nesting a bit? you can invert some if statements, and do newWalls.Add(wall); return if they are true. flatter code is easier to read/reason about I think. sometimes I think of the first part of a method like "reasons not to do the normal thing" and then "the normal thing" below those checks.

wynged

Reviewed 1 of 2 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 5 unresolved discussions (waiting on @jamesbradleym)

jamesbradleym

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 5 unresolved discussions (waiting on @wynged)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 1 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

what's this for?

Removed, not sure what added that.

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 31 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

doesn't matter much, but I might make the method below create the ordered list of levels internally rather than requiring that the levels being passed in be sorted. might prevent a mistake at some future date if we mis-order the levels or order them in some other way for some other reason, the SplitWallsByLevels method could just keep working if it did it all internally.

Moved to SplitWallsByLevel

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 60 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

making the walls argument be for IEnumerable<StandardWall> would make you not need to do ToList above.

👍🏻

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

what is 0.1?

This was a hard coded wall thickness value. Maybe it would be valuable to also group walls by thickness prior to processing, after all we wouldn't want to merge walls of different thicknesses. Pringle has a set value for all wall thicknesses currently but that would be a reasonable update to make. (updated it to .13335 which is the actual pringle value but I'll see about grouping by thickness and then iterating through those values and reapplying for made walls)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 103 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

can we reduce the nesting a bit? you can invert some if statements, and do newWalls.Add(wall); return if they are true. flatter code is easier to read/reason about I think. sometimes I think of the first part of a method like "reasons not to do the normal thing" and then "the normal thing" below those checks.

Done!

wynged

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 2 unresolved discussions (waiting on @jamesbradleym)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

Previously, jamesbradleym wrote…

This was a hard coded wall thickness value. Maybe it would be valuable to also group walls by thickness prior to processing, after all we wouldn't want to merge walls of different thicknesses. Pringle has a set value for all wall thicknesses currently but that would be a reasonable update to make. (updated it to .13335 which is the actual pringle value but I'll see about grouping by thickness and then iterating through those values and reapplying for made walls)

yea, I think grouping by thickness makes sense. even right now I think someone could technically end up with two different thicknesses if they changed the units of their project right? if not, mabye this is fine for now.

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 65 at r2 (raw file):

        public static List<StandardWall> SplitWallsByLevels(IEnumerable<StandardWall> walls, List<Level> levels, Random random)
        {
            levels.Sort((level1, level2) => level1.Elevation.CompareTo(level2.Elevation));

I think make a new sortedLevels enumerable rather than sorting the incoming one. sorting this incoming list would qualify as a side effect which isn't normally desirable unless done for a specific reason.

jamesbradleym

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 2 unresolved discussions (waiting on @wynged)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

yea, I think grouping by thickness makes sense. even right now I think someone could technically end up with two different thicknesses if they changed the units of their project right? if not, mabye this is fine for now.

Yep, I think that's possible. We're using wallThickness: 0.135, in defaultValuesMetric and 5.25" in defaultValuesImperial (0.13335m). The group by thickness change would resolve this discrepancy (keeping these walls from merging, maybe we want a fuzziness/tolerance here as well..)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 65 at r2 (raw file):

Previously, wynged (Eric Wassail) wrote…

I think make a new sortedLevels enumerable rather than sorting the incoming one. sorting this incoming list would qualify as a side effect which isn't normally desirable unless done for a specific reason.

gotcha, misunderstood - made a new list from ordered levels

wynged

Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, all discussions resolved

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

Previously, jamesbradleym wrote…

Yep, I think that's possible. We're using wallThickness: 0.135, in defaultValuesMetric and 5.25" in defaultValuesImperial (0.13335m). The group by thickness change would resolve this discrepancy (keeping these walls from merging, maybe we want a fuzziness/tolerance here as well..)

yea, let's give a tolerance, let's say 1mm I think, that's bigger than just a numeric EPSILON but seems quite reasonable for walls. Your call if you want to do this now though. we don't actualy let users change their wall thickness, so we could deal later if we wanted.

wynged

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 1 of 1 approvals obtained, all discussions resolved

jamesbradleym

Reviewable status: complete! 1 of 1 approvals obtained, all discussions resolved

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

Previously, wynged (Eric Wassail) wrote…

yea, let's give a tolerance, let's say 1mm I think, that's bigger than just a numeric EPSILON but seems quite reasonable for walls. Your call if you want to do this now though. we don't actualy let users change their wall thickness, so we could deal later if we wanted.

Wouldn't mind getting your thoughts on the tolerance grouping.

It forces everything to check equality with tolerance rather than the hash which moves it to O(n) rather than the hash O(1) but given how quick Math.Abs(x - y) <= _tolerance; is that seems fine?

jamesbradleym

per conversation with Eric & Kat -

Updated to migrate values from/to default metric to/from default imperial based on Project Settings unit system

Reviewable status: complete! 1 of 1 approvals obtained, all discussions resolved

wynged

Reviewed 1 of 2 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 2 unresolved discussions (waiting on @jamesbradleym)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 87 at r1 (raw file):

Previously, jamesbradleym wrote…

Wouldn't mind getting your thoughts on the tolerance grouping.

It forces everything to check equality with tolerance rather than the hash which moves it to O(n) rather than the hash O(1) but given how quick Math.Abs(x - y) <= _tolerance; is that seems fine?

seems ok, there will be issues with kind of bucketing, but the unit system approach should help us do more reasonable things in the future.

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 24 at r5 (raw file):

            if (inputModels.TryGetValue("Project Settings", out var settingsModel))
            {
                foreach (var elementDict in settingsModel.Elements)

we should probably find the single element by discriminator rather than looking at all elements.

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 39 at r5 (raw file):

            {
                var walls = wallsModel.AllElementsOfType<StandardWall>();
                if (unitSystem != null)

can unit system ever equal null, you set i to metric above.

…null before setting

jamesbradleym

Reviewable status: 1 change requests, 0 of 1 approvals obtained, 2 unresolved discussions (waiting on @wynged)

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 24 at r5 (raw file):

Previously, wynged (Eric Wassail) wrote…

we should probably find the single element by discriminator rather than looking at all elements.

Gotcha, changed to find the first element with ["discriminator"] = Elements.ProjectSettings

LayoutFunctions/WallsLOD200/src/WallsLOD200.cs line 39 at r5 (raw file):

Previously, wynged (Eric Wassail) wrote…

can unit system ever equal null, you set i to metric above.

The unitSystem from the model is coming back as a string?, so this is a safety check, moved it to the moment of setting rather than here.

wynged

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained, all discussions resolved

support multi-level walls

cc3c20a

jamesbradleym requested a review from wynged July 25, 2024 13:36

wynged requested changes Jul 25, 2024

View reviewed changes

wynged reviewed Jul 25, 2024

View reviewed changes

comments

c79b5f7

jamesbradleym commented Jul 25, 2024

View reviewed changes

jamesbradleym requested a review from wynged July 25, 2024 16:18

group by thickness and apply

088c0f6

wynged requested changes Jul 26, 2024

View reviewed changes

don't modify original levels

cbc8091

jamesbradleym commented Jul 26, 2024

View reviewed changes

wynged reviewed Jul 26, 2024

View reviewed changes

wynged approved these changes Jul 26, 2024

View reviewed changes

add thickness tolerance grouping

5f9b95e

jamesbradleym commented Jul 26, 2024

View reviewed changes

set walls to default thickness by unit system

3868672

jamesbradleym commented Jul 30, 2024

View reviewed changes

jamesbradleym requested a review from wynged July 30, 2024 22:04

wynged requested changes Jul 31, 2024

View reviewed changes

look for settings by discriminator and ensure unitsystem found isn't …

e50ea73

…null before setting

jamesbradleym commented Jul 31, 2024

View reviewed changes

jamesbradleym requested a review from wynged July 31, 2024 15:18

wynged approved these changes Jul 31, 2024

View reviewed changes

jamesbradleym merged commit 26bbacf into master Jul 31, 2024
1 check passed

jamesbradleym deleted the multi-level-walls branch July 31, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support multi-level walls #103

support multi-level walls #103

jamesbradleym commented Jul 25, 2024 •

edited by andrewheumann

Loading

wynged left a comment

wynged left a comment

jamesbradleym left a comment

wynged left a comment

jamesbradleym left a comment

wynged left a comment

wynged left a comment

jamesbradleym left a comment

jamesbradleym left a comment

wynged left a comment

jamesbradleym left a comment

wynged left a comment

support multi-level walls #103

support multi-level walls #103

Conversation

jamesbradleym commented Jul 25, 2024 • edited by andrewheumann Loading

wynged left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

jamesbradleym left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

jamesbradleym left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

jamesbradleym left a comment

Choose a reason for hiding this comment

jamesbradleym left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

jamesbradleym left a comment

Choose a reason for hiding this comment

wynged left a comment

Choose a reason for hiding this comment

jamesbradleym commented Jul 25, 2024 •

edited by andrewheumann

Loading