-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Layout Function Cleanup #37
Conversation
bump @wynged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 16 of 20 files at r1, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 3 unresolved discussions (waiting on @andrewheumann, @nadiia-volyk, and @wynged)
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 50 at r1 (raw file):
foreach (var lvl in levels) { var corridors = lvl.Elements.Where(e => e is Floor).OfType<Floor>();
I'm not as familiar with this code, so I'm curious why this was changed from lvl.Elements.OfType<CirculationSegment>()
?
Code quote:
lvl.Elements.Where(e => e is Floor).OfType<Floor>()
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 243 at r1 (raw file):
} protected virtual IEnumerable<TLevelElements> GetLevels(Dictionary<string, Model> inputModels, Model spacePlanningZones)
Can we add a comment that GetLevels
also assigns the relevant circulation segments to those levels?
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 276 at r1 (raw file):
} return levelVolumes; }
This method already seems to exist in LayoutStrategies
, do we need to replicate it here?
Code quote:
private static List<TLevelVolume> GetLevelVolumes(Dictionary<string, Model> inputModels)
{
var levelVolumes = new List<TLevelVolume>();
if (inputModels.TryGetValue("Levels", out var levelsModel))
{
levelVolumes.AddRange(levelsModel.AllElementsAssignableFromType<TLevelVolume>());
}
if (inputModels.TryGetValue("Conceptual Mass", out var massModel))
{
levelVolumes.AddRange(massModel.AllElementsAssignableFromType<TLevelVolume>());
}
return levelVolumes;
}
# Conflicts: # LayoutFunctions/ClassroomLayout/dependencies/ClassroomLayout.Dependencies.csproj # LayoutFunctions/MeetingRoomLayout/dependencies/MeetingRoomLayout.Dependencies.csproj # LayoutFunctions/MeetingRoomLayout/src/MeetingRoomLayout.cs # LayoutFunctions/OpenCollabLayout/src/OpenCollaborationLayout.cs # LayoutFunctions/PantryLayout/dependencies/PantryLayout.Dependencies.csproj # LayoutFunctions/PantryLayout/src/PantryLayout.cs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 3 unresolved discussions (waiting on @andrewheumann, @serenayl, and @wynged)
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 50 at r1 (raw file):
Previously, serenayl (Serena Li) wrote…
I'm not as familiar with this code, so I'm curious why this was changed from
lvl.Elements.OfType<CirculationSegment>()
?
This code was from LayoutStrategies. I don't see reason for it. Changed it to TCirculationSegment
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 243 at r1 (raw file):
Previously, serenayl (Serena Li) wrote…
Can we add a comment that
GetLevels
also assigns the relevant circulation segments to those levels?
Done.
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 276 at r1 (raw file):
Previously, serenayl (Serena Li) wrote…
This method already seems to exist in
LayoutStrategies
, do we need to replicate it here?
No. Removed it
- Add virtual method to LayoutGeneration to provide custom order of preferred configs
My test workflow - https://hypar.io/workflows/1d11bd4a-5c93-45ff-92b8-a9ac11d5664b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 19 of 20 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 3 unresolved discussions (waiting on @serenayl and @wynged)
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 50 at r1 (raw file):
Previously, nadiia-volyk wrote…
This code was from LayoutStrategies. I don't see reason for it. Changed it to TCirculationSegment
This is a good change, anything treating circulation as a floor is old / out of date :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewheumann @serenayl documentation of these changes is here. Look for New #2, looks pretty good to me! https://www.notion.so/hyparaec/Document-layout-differences-075a152f778247848d05518773c69050?pvs=4
Reviewed 9 of 20 files at r1.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 3 unresolved discussions (waiting on @serenayl)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The layout mostly looks good but it looks like we've lost our Collaboration Seat
count. Before and after:
Reviewed 14 of 17 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 1 unresolved discussion (waiting on @nadiia-volyk)
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 94 at r3 (raw file):
totalSeats += seatsCount; outputModel.AddElement(new SpaceMetric(room.Id, seatsCount, 0, 0, 0));
See here for where I suspect we are losing seats.
Code quote:
outputModel.AddElement(new SpaceMetric(room.Id, seatsCount, 0, 0, 0))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, 1 unresolved discussion (waiting on @serenayl)
LayoutFunctions/LayoutFunctionCommon/LayoutGeneration.cs
line 94 at r3 (raw file):
Previously, serenayl (Serena Li) wrote…
See here for where I suspect we are losing seats.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained, all discussions resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 1 of 1 approvals obtained, all discussions resolved
Move duplicated code to LayoutGeneration class with a few virtual methods that are overridden in specific layout functions
Differences in layouts after cleanup are here
This change is