Implementation of priority sectors #240
Merged
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
Strepto
reviewed
Sep 10, 2024
Comment on lines
162
to
164
| // TODO: Should these be a part of protobuf? | ||
| public int Priority { get; init; } = 0; | ||
| public string? Discipline { get; init; } = null; |
There was a problem hiding this comment.
Protobuf is for everything you want to have working in the "Fast-Split resume" functionality, so this should possibly be there.
Should maybe consider "compressing" discipline to a reference object instead of repeating 10 different strings X1000 times. Edit: I think dotnet may handle string duplication augomagically for us, so may not be needed.
Strepto
reviewed
Sep 26, 2024
| using System.Linq; | ||
| using Newtonsoft.Json; | ||
|
|
||
| public static class StidTagMapper |
There was a problem hiding this comment.
Leave the StidTagMapper stuff to another PR, this needs to come in separately
Draft
stigrus
reviewed
Oct 28, 2024
vegasten
commented
Oct 28, 2024
CadRevealComposer/Operations/SectorSplitting/PrioritySplittingUtils.cs
Outdated
Show resolved
Hide resolved
CadRevealComposer.Tests/Operations/Splitting/PrioritySplittingUtilsTests.cs
Outdated
Show resolved
Hide resolved
CadRevealComposer/Operations/SectorSplitting/PrioritySectorSplitter.cs
Outdated
Show resolved
Hide resolved
stigrus
reviewed
Oct 29, 2024
Strepto
reviewed
Oct 30, 2024
vegasten
commented
Oct 30, 2024
CadRevealComposer/Operations/SectorSplitting/PrioritySectorSplitter.cs
Outdated
Show resolved
Hide resolved
vegasten
commented
Oct 30, 2024
CadRevealComposer/Operations/SectorSplitting/PrioritySectorSplitter.cs
Outdated
Show resolved
Hide resolved
vegasten
commented
Oct 30, 2024
vegasten
commented
Oct 30, 2024
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.
(Renamed stuff from "highlighting" to "priority" to make the functionality itself less specific.)
Creating of prioritized sectors and adding a table for "treeindex - priority sector" in the hierarchyDB.
Creation of sectors
Includes a custom splitter for priority sectors, which is simpler than the usual splitter. It also has an independent budget, making it possible to smaller sectors.
Budget might need some tweaking, but generally be as small as possible without creating too many sectors. Hence, dependent on how many parts are included in the "prioritized parts".
=> Including more disciplines, etc. also requires modifying the budget accordingly
HierarchyDB
The current solution breaks parallelization by needing to wait for both splitting and hierarchy before adding the new data. Adding the new data is luckily quite quick (~1 second on Troll A), so the total time is not noticably different. It's more a question of code quality and increased complexity..
Note: Some TODOs are still present and in progress.