Skip to content

Add 'fit to scene' feature to examples/viewer#425

Open
Copilot wants to merge 4 commits into
releasefrom
copilot/add-fit-to-scene-feature
Open

Add 'fit to scene' feature to examples/viewer#425
Copilot wants to merge 4 commits into
releasefrom
copilot/add-fit-to-scene-feature

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 27, 2026

  • Inspect PR comments and identify actionable feedback in the linked review thread
  • Review current Init()/FitToScene() implementation in examples/viewer/viewer.cc
  • Run baseline build/tests for the repository before code changes
  • Apply a minimal refactor to keep camera reset logic in one place
  • Run targeted validation after the change
  • Run final automated validation and reply to addressed comment

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

- Press 'F' key to reset camera to view entire model
- Initial camera position set to fit the scene on load
- Enable centering translation to center objects at origin
- Store bounding box data in globals for FitToScene() access
- Update help text to document 'F' key

Agent-Logs-Url: https://github.com/tinyobjloader/tinyobjloader/sessions/3b05e7bf-a2b5-4b2d-b308-718537dfc75a

Co-authored-by: syoyo <18676+syoyo@users.noreply.github.com>
Copilot AI changed the title [WIP] Add fit to scene feature to examples/viewer Add 'fit to scene' feature to examples/viewer Mar 27, 2026
Copilot AI requested a review from syoyo March 27, 2026 19:51
@syoyo syoyo marked this pull request as ready for review March 27, 2026 20:03
Copilot AI review requested due to automatic review settings March 27, 2026 20:03
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a “fit to scene” camera reset to the OpenGL OBJ viewer example, and fixes initial framing by centering/scaling the loaded model consistently.

Changes:

  • Introduces FitToScene() and binds it to the F key; also calls it after loading to set a sensible initial view.
  • Persists scene bounds (scene_bmin/scene_bmax) and scene_maxExtent at file scope for use during rendering and fit/reset.
  • Enables the previously-disabled centering translation and uses the persisted bounds/extent in the render loop.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread examples/viewer/viewer.cc
Comment on lines +1118 to +1136
// Reset camera to fit the entire scene in view.
static void FitToScene() {
trackball(curr_quat, 0, 0, 0, 0);

g_angleX = 0.0f;
g_angleY = 0.0f;

eye[0] = 0.0f;
eye[1] = 0.0f;
eye[2] = 3.0f;

lookat[0] = 0.0f;
lookat[1] = 0.0f;
lookat[2] = 0.0f;

up[0] = 0.0f;
up[1] = 1.0f;
up[2] = 0.0f;
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

FitToScene() duplicates the camera reset logic already in Init() (same quaternion + eye/lookat/up defaults) with only small differences. To avoid the two getting out of sync over time, consider having Init() call FitToScene() (or vice versa) and keep the reset logic in one place.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 98840c2: Init() now delegates to FitToScene(), so camera reset defaults are maintained in one place and won’t drift.

Comment thread examples/viewer/viewer.cc
Comment on lines +1189 to +1190
scene_bmin[0] = bmin[0]; scene_bmin[1] = bmin[1]; scene_bmin[2] = bmin[2];
scene_bmax[0] = bmax[0]; scene_bmax[1] = bmax[1]; scene_bmax[2] = bmax[2];
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

The chained assignments for scene_bmin/scene_bmax on single lines reduce readability and make diffs harder to review. Consider formatting these as one assignment per line (or using std::copy) for consistency with the surrounding style.

Suggested change
scene_bmin[0] = bmin[0]; scene_bmin[1] = bmin[1]; scene_bmin[2] = bmin[2];
scene_bmax[0] = bmax[0]; scene_bmax[1] = bmax[1]; scene_bmax[2] = bmax[2];
scene_bmin[0] = bmin[0];
scene_bmin[1] = bmin[1];
scene_bmin[2] = bmin[2];
scene_bmax[0] = bmax[0];
scene_bmax[1] = bmax[1];
scene_bmax[2] = bmax[2];

Copilot uses AI. Check for mistakes.
Comment thread examples/viewer/viewer.cc
Comment on lines +1228 to +1229
glScalef(1.0f / scene_maxExtent, 1.0f / scene_maxExtent,
1.0f / scene_maxExtent);
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

scene_maxExtent can become 0 (e.g., degenerate geometry where bmin==bmax on all axes), which would make glScalef(1.0f / scene_maxExtent, ...) divide by zero and propagate INF/NaN through the modelview matrix. Consider clamping scene_maxExtent to a small positive epsilon (or falling back to 1.0f) after computing it, and/or early-out when bounds are invalid.

Suggested change
glScalef(1.0f / scene_maxExtent, 1.0f / scene_maxExtent,
1.0f / scene_maxExtent);
float scale = 1.0f;
if (scene_maxExtent > 0.0f) {
scale = 1.0f / scene_maxExtent;
}
glScalef(scale, scale, scale);

Copilot uses AI. Check for mistakes.
Copilot finished work on behalf of syoyo May 20, 2026 18:00
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.

3 participants