Conversation
ragnar-howler
approved these changes
Mar 8, 2026
ragnar-howler
left a comment
There was a problem hiding this comment.
Code Review: Removed static variables
Summary
This PR addresses an important architectural issue where static variables with thread_local work correctly in the native app but fail in glvis-js, where new visualization scene instances are started in the same window/thread without reinitialization.
Changes Analysis
1. Static Member Migration (Good)
- Moving
vsdata,vssol,vssol3d,vsvector,vsvector3dfrom global statics tostatic thread_localclass members is proper encapsulation - Thread safety is preserved -
thread_localensures per-thread isolation
2. Key Handler Refactoring (Good)
- Converting free functions to
staticmember functions improves code organization - Making them
privatehides implementation details from public API - All key handlers properly access scene instance via the static thread_local pointer
3. Instance Variable Fixes (Critical Fix)
Moving these from static globals to instance variables correctly fixes state carryover:
magic_key_pressed(vssolution3d)ianim,ianimmax,key_u_func(vsvector)arrows_nl,vector_h,vector_hh(vsvector3d - replaces static locals)
This ensures each scene instance maintains its own state.
4. Plane Constructor Change
// Before:
Plane(double A, double B, double C, double D);
// After:
Plane(const double (&eqn_)[4], const VisualizationScene::Box &bb);- Passing bounding box by reference avoids accessing global state
- Array reference for equation parameters is cleaner
- The named struct
Boxinopenglvis.hppenables this
5. Logic Corrections
vsvector.cpp:ChangeDisplacement()correctly handles wraparound with modulo arithmeticvsvector3d.cpp:ArrowsDrawOrNot()now usesarrows_nlmember (initialized to -1) instead offirst_timestaticKeyrPressed/KeyRPressednow correctly usevsvector3d->instead ofwindow->vs->
Verdict
✅ Approve
This is a well-executed refactoring that:
- Fixes the glvis-js state carryover bug
- Improves encapsulation
- Maintains thread safety
- Preserves all existing functionality
The changes are minimal and focused on the specific issue without unnecessary scope creep.
This file contains hidden or 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
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.
Refactored code to remove static variables from
VisualizationScenes, which are fine in the native app when declared asthread_local(not always the case), but fail in glvis-js, where a new instance of visualization scene is started (in the same window and thread). Initialization does not happen again and the values are carried over.Bonus: Changed key handlers and variables to private.