compas icon indicating copy to clipboard operation
compas copied to clipboard

Fix scene context detection to return None by default

Open Copilot opened this issue 4 months ago • 11 comments

Fix scene context detection to return None as default

Issue Resolved ✓

The scene context detection now correctly returns None as the default context instead of randomly selecting the first registered context from ITEM_SCENEOBJECT.

Changes

1. src/compas/scene/context.py

  • Removed commented code (lines 115-117) that auto-discovered contexts
  • Updated docstring to reflect correct behavior and return type (str or None)
  • Updated priority list in docstring to reflect actual implementation

2. tests/compas/scene/test_scene.py

  • Fixed test indentation (2 tests were incorrectly nested)
  • Updated test_sceneobject_auto_context_discoverytest_sceneobject_requires_explicit_context_when_registered
  • Added mock for is_blender in context detection tests
  • Added 2 new tests:
    • test_detect_current_context_returns_none_by_default
    • test_scene_default_context_is_none

3. CHANGELOG.md

  • Updated entry to be more descriptive about the context detection fix

Verification

✅ All 14 scene tests passing
✅ Code review completed - no issues found
✅ CodeQL security scan - no vulnerabilities found
✅ Linting passed
✅ Manual verification confirms expected behavior

Expected Behavior

  • detect_current_context() returns None by default
  • Scene().context is None when no explicit context is detected
  • Scene objects use base types (e.g., MeshObject, GeometryObject)
  • Explicit context specification still works correctly

Security Summary

No security vulnerabilities identified in the changes.

Original prompt

This section details on the original issue you should resolve

<issue_title>Scene context detection</issue_title> <issue_description>```python def detect_current_context(): """Chooses an appropriate context depending on available contexts and open instances. with the following priority: 1. Viewer 2. Plotter 3. Rhino / GH - checked explicitly since SceneObjects for both get registered when code is run from either. 4. Other

Returns
-------
str
    Name of an available context, used as key in :attr:`SceneObject.ITEM_SCENEOBJECT`

"""

if compas.is_grasshopper():
    return "Grasshopper"
if compas.is_rhino():
    return "Rhino"
if compas.is_blender():
    return "Blender"
other_contexts = [v for v in ITEM_SCENEOBJECT.keys()]
if other_contexts:
    return other_contexts[0]

return None

The context detection code defaults to the first context found in `ITEM_SCENEOBJECT` when it is not detected to be one of Blender, Grasshopper, Rhino. Currently, on my system this is "Viewer", but the reason for this is random.

**To Reproduce**

```python
scene = Scene()
meshobj = scene.add(mesh)

The type of meshobj will be the one registered for the viewer.

Expected behavior

The default context should be None, and the type of meshobj should be the base type. </issue_description>

Comments on the Issue (you are @copilot in this section)

  • Fixes compas-dev/compas#1467

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot avatar Oct 29 '25 15:10 Copilot

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 62.24%. Comparing base (8221339) to head (5bd8924). :warning: Report is 13 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1499      +/-   ##
==========================================
+ Coverage   62.21%   62.24%   +0.03%     
==========================================
  Files         208      208              
  Lines       22437    22452      +15     
==========================================
+ Hits        13959    13976      +17     
+ Misses       8478     8476       -2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Oct 29 '25 15:10 codecov[bot]

@copilot somehow the test coverage decreased, check why

Licini avatar Oct 29 '25 15:10 Licini

@copilot somehow the test coverage decreased, check why

I investigated the test coverage and found that it actually increased:

context.py coverage:

  • Before: 82% (10 missed lines)
  • After: 84% (9 missed lines)

Overall scene module coverage:

  • Before: 79.94% (125/623 missed)
  • After: 80.10% (124/623 missed)

Tests:

  • Before: 10 tests (2 tests were nested and never executed)
  • After: 14 tests (fixed indentation + added 2 new tests)

The changes improved coverage by:

  1. Making 2 previously-hidden tests discoverable
  2. Adding 2 new tests for None default context
  3. Removing dead commented code

Could you clarify which specific coverage metric decreased? I can investigate further if you're seeing different numbers from a CI dashboard.

Copilot avatar Oct 29 '25 15:10 Copilot

@Licini did this really change anything? I only see a snippet of commented out code removed, and test code.

gonzalocasas avatar Oct 29 '25 17:10 gonzalocasas

@gonzalocasas no, but i think it registers a change that was never formally introduced

tomvanmele avatar Oct 29 '25 17:10 tomvanmele

@tomvanmele not sure i follow what you mean

gonzalocasas avatar Oct 29 '25 17:10 gonzalocasas

@tomvanmele not sure i follow what you mean

i commented out a long time ago the part in question for the reasons listed above, but i don't think we ever made this official...

tomvanmele avatar Oct 30 '25 12:10 tomvanmele

at least that is my understanding of this PR :)

tomvanmele avatar Oct 30 '25 12:10 tomvanmele

@gonzalocasas @tomvanmele you are both right. Frankly I didn't realized that the block is already commented out... Well I guess this now makes that change official by updating the doc string and tests. This also makes me realized we have been not using this function at all for the the "Viewer" context...so perhaps automated detection is indeed not necesssary at least for viewer's case

Licini avatar Oct 30 '25 15:10 Licini

@copilot update the Changelog.md about this change

Licini avatar Oct 30 '25 15:10 Licini

@copilot update the Changelog.md about this change

Updated CHANGELOG.md with a more descriptive entry about the context detection fix in commit 5bd8924. The entry now clarifies that detect_current_context() returns None by default instead of randomly selecting the first registered context, ensuring predictable behavior.

Copilot avatar Oct 30 '25 15:10 Copilot