Skip to content
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

Visible/Non Visible Specifier Changes #214

Merged
merged 33 commits into from
Apr 19, 2024
Merged

Conversation

Eric-Vin
Copy link
Collaborator

@Eric-Vin Eric-Vin commented Feb 2, 2024

The visible/non visible specifiers now specify position considering the whole occupied space rather than just the position. For example, this means visible can place objects that have a corner visible, and not visible can place objects that are behind walls.

@Eric-Vin Eric-Vin requested a review from dfremont February 2, 2024 03:21
@Eric-Vin Eric-Vin marked this pull request as draft February 2, 2024 03:35
@Eric-Vin Eric-Vin marked this pull request as ready for review February 2, 2024 04:14
@Eric-Vin
Copy link
Collaborator Author

Eric-Vin commented Feb 2, 2024

Going to wait for #213 to be merged in, so I can merge in main and fix the formatting check.

docs/porting.rst Outdated Show resolved Hide resolved
src/scenic/core/regions.py Outdated Show resolved Hide resolved
src/scenic/core/regions.py Outdated Show resolved Hide resolved
@Eric-Vin Eric-Vin marked this pull request as draft February 9, 2024 01:23
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 97.49104% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 86.15%. Comparing base (d5a21fc) to head (35fa757).
Report is 4 commits behind head on main.

Files Patch % Lines
src/scenic/core/regions.py 94.20% 4 Missing ⚠️
src/scenic/core/pruning.py 96.77% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   85.89%   86.15%   +0.26%     
==========================================
  Files         144      144              
  Lines       24767    24938     +171     
==========================================
+ Hits        21274    21486     +212     
+ Misses       3493     3452      -41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/scenic/core/regions.py Outdated Show resolved Hide resolved
src/scenic/core/regions.py Outdated Show resolved Hide resolved
src/scenic/core/pruning.py Outdated Show resolved Hide resolved
src/scenic/core/pruning.py Outdated Show resolved Hide resolved
src/scenic/core/pruning.py Outdated Show resolved Hide resolved
src/scenic/core/pruning.py Outdated Show resolved Hide resolved
tests/core/test_regions.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dfremont dfremont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall: a few comments above, but mostly minor.

One other thing that I couldn't leave as a comment: the docs for the visible and not visible specifiers should now list regionContainedIn as a dependency. (Arguably it's an implementation detail, not really part of the semantics, but it is a real dependency so we should probably document it.)

@Eric-Vin Eric-Vin requested a review from dfremont April 16, 2024 23:47
Copy link
Collaborator

@dfremont dfremont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just a few last nitpicks.

@Eric-Vin Eric-Vin requested a review from dfremont April 18, 2024 18:37
Copy link
Collaborator

@dfremont dfremont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great -- thanks Eric for persevering through this long review process!

@Eric-Vin Eric-Vin merged commit 7f018e0 into main Apr 19, 2024
26 checks passed
@Eric-Vin Eric-Vin deleted the VisibleSpecifierChanges branch April 19, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants