feat: Identifier can be customized for sensitive surfaces #1452
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
|
So, if we had lets say 20 surfaces front size and 20 surfaces backside, would you do 0-20 and a side bit, but 0-40 and a side bit ? |
Codecov Report
@@ Coverage Diff @@
## main #1452 +/- ##
==========================================
+ Coverage 48.59% 48.60% +0.01%
==========================================
Files 380 380
Lines 20589 20595 +6
Branches 9431 9433 +2
==========================================
+ Hits 10005 10010 +5
Misses 4097 4097
- Partials 6487 6488 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
I addressed @andiwand's comments. Can one of you approve again? |
andiwand
approved these changes
Aug 22, 2022
paulgessinger
added
automerge
backport develop/v19.x
|
Hmm. I didn't see hash failures locally... |
|
The fast-test right after FATRAS only already fails. I'll have to investigate. I'm taking this WIP again. |
AJPfleger
reviewed
Aug 25, 2022
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
|
Ok I think the hash changes come from the fact that we write the actual geometry id 64bit integer value to the outputs, which clearly changes if we reduce the bit width of the sensitive id. Can you reapprove @andiwand? |
andiwand
approved these changes
Aug 31, 2022
acts-project-service
pushed a commit
that referenced
this pull request
Aug 31, 2022
This PR reduces the width of the sensitive part of the geometry identifier from 28bits to 20bits, to make room for an 8bit *extra* identifier part. The idea is that this can be used by experiments to label sensitive elements. The bit masks after the change look like: ```cpp static constexpr Value kVolumeMask = 0xff00000000000000; // (2^8)-1 = 255 volumes static constexpr Value kBoundaryMask = 0x00ff000000000000; // (2^8)-1 = 255 boundaries static constexpr Value kLayerMask = 0x0000fff000000000; // (2^12)-1 = 4096 layers static constexpr Value kApproachMask = 0x0000000ff0000000; // (2^8)-1 = 255 approach surfaces static constexpr Value kSensitiveMask = 0x000000000fffff00; // (2^20)-1 sensitive surfaces static constexpr Value kExtraMask = 0x00000000000000ff; // (2^8)-1 extra values ``` In addition, it adds a hook function to the geometry closure of signature ```cpp GeometryIdentifier(GeometryIdentifier orig, const Surface& surface); ``` to customize the geometry identifier. Technically, this hook can be used to also reset the volume and layer ids, but is primarily intended to set the *extra* components. (cherry picked from commit a5df559)
kodiakhq bot
pushed a commit
that referenced
this pull request
Aug 31, 2022
…1452 to develop/v19.x] (#1479) Backport a5df559 from #1452. --- This PR reduces the width of the sensitive part of the geometry identifier from 28bits to 20bits, to make room for an 8bit *extra* identifier part. The idea is that this can be used by experiments to label sensitive elements. The bit masks after the change look like: ```cpp static constexpr Value kVolumeMask = 0xff00000000000000; // (2^8)-1 = 255 volumes static constexpr Value kBoundaryMask = 0x00ff000000000000; // (2^8)-1 = 255 boundaries static constexpr Value kLayerMask = 0x0000fff000000000; // (2^12)-1 = 4096 layers static constexpr Value kApproachMask = 0x0000000ff0000000; // (2^8)-1 = 255 approach surfaces static constexpr Value kSensitiveMask = 0x000000000fffff00; // (2^20)-1 sensitive surfaces static constexpr Value kExtraMask = 0x00000000000000ff; // (2^8)-1 extra values ``` In addition, it adds a hook function to the geometry closure of signature ```cpp GeometryIdentifier(GeometryIdentifier orig, const Surface& surface); ``` to customize the geometry identifier. Technically, this hook can be used to also reset the volume and layer ids, but is primarily intended to set the *extra* components.
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 PR reduces the width of the sensitive part of the geometry identifier from 28bits to 20bits, to make room for an 8bit extra identifier part. The idea is that this can be used by experiments to label sensitive elements. The bit masks after the change look like:
In addition, it adds a hook function to the geometry closure of signature
to customize the geometry identifier. Technically, this hook can be used to also reset the volume and layer ids, but is primarily intended to set the extra components.