Removed static variables

[najlkin]

Refactored code to remove static variables from VisualizationScenes, which are fine in the native app when declared as thread_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.

[camierjs]

Thank you @najlkin, neat!

[ragnar-howler]

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, vsvector3d from global statics to static thread_local class members is proper encapsulation
• Thread safety is preserved - thread_local ensures per-thread isolation

2. Key Handler Refactoring (Good)

• Converting free functions to static member functions improves code organization
• Making them private hides 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 Box in openglvis.hpp enables this

5. Logic Corrections

• vsvector.cpp: ChangeDisplacement() correctly handles wraparound with modulo arithmetic
• vsvector3d.cpp: ArrowsDrawOrNot() now uses arrows_nl member (initialized to -1) instead of first_time static
• KeyrPressed/KeyRPressed now correctly use vsvector3d-> instead of window->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.

[Copilot]

Pull request overview

This PR refactors GLVis VisualizationScene* implementations to eliminate function-local static state that can persist incorrectly across scene instances (notably in glvis-js), and restructures key handlers/state to be scene-owned (and mostly private).

Changes:

• Moved several scene-specific “static” caches/state into per-instance members (e.g., vector field scaling, arrow selection state).
• Converted many free/static key handlers into static class members with a thread_local “current scene” pointer.
• Introduced VisualizationScene::Box type and updated Plane construction to take a bounding box explicitly.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
lib/vsvector3d.hpp	Adds per-instance fields and privatizes key handler plumbing for 3D vector scenes.
lib/vsvector3d.cpp	Removes function-local statics; moves vector scaling/arrow logic to instance state; converts key handlers to class statics.
lib/vsvector.hpp	Moves key handler state into the class and adjusts member layout (e.g., ArrowScale).
lib/vsvector.cpp	Converts key handlers to class statics; replaces thread_local globals with members; initializes per-instance key state in Init().
lib/vssolution3d.hpp	Moves key handler declarations/state into the class and makes them non-public.
lib/vssolution3d.cpp	Converts free key handlers into class statics; updates cutting plane creation to use new Plane ctor.
lib/vssolution.hpp	Moves key handler declarations/state into the class and makes them non-public; adds SwitchAttribute helper as a member.
lib/vssolution.cpp	Converts free key handlers into class statics; makes SwitchAttribute a member; updates cutting plane creation to use new Plane ctor.
lib/vsdata.hpp	Changes Plane API; moves CuttingPlane/shrink factors into protected; adds key handler declarations/state.
lib/vsdata.cpp	Converts free key handlers into class statics; updates Plane ctor implementation to use explicit bounding box input.
lib/openglvis.hpp	Names the bounding box struct as VisualizationScene::Box for reuse outside VisualizationScene.

---

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

[Copilot]

The new Plane constructor takes const double (&)[4], but this call passes a braced-init-list. In standard C++, {...} will not bind to a reference-to-array parameter, so this is likely a compile error. Consider changing the constructor to accept a type that can be constructed from a braced list (e.g., std::array<double,4> / mfem::Vector / std::span<const double,4>), or create a named double eqn[4] array and pass that.

Suggested change

	CuttingPlane = new Plane({-1.0,0.0,0.0,(0.5-eps)*bb.x[0]+(0.5+eps)*bb.x[1]},bb);
	double cutting_plane_eqn[4] =
	{
	-1.0, 0.0, 0.0, (0.5-eps)*bb.x[0] + (0.5+eps)*bb.x[1]
	};
	CuttingPlane = new Plane(cutting_plane_eqn, bb);

[Copilot]

The new Plane constructor takes const double (&)[4], but this call passes a braced-init-list. In standard C++, {...} will not bind to a reference-to-array parameter, so this is likely a compile error. Consider changing the constructor to accept a type that can be constructed from a braced list (e.g., std::array<double,4> / mfem::Vector / std::span<const double,4>), or create a named double eqn[4] array and pass that.

Suggested change

	CuttingPlane = new Plane({-1.0,0.0,0.0,(0.5-eps)*bb.x[0]+(0.5+eps)*bb.x[1]},bb);
	double cutting_plane_eqn[4] =
	{
	-1.0, 0.0, 0.0, (0.5-eps)*bb.x[0] + (0.5+eps)*bb.x[1]
	};
	CuttingPlane = new Plane(cutting_plane_eqn, bb);

[Copilot]

ArrowsDrawOrNot now returns early when arrows_nl == nl, but PrepareVectorField() allocates a fresh Array<int>[nl+1] and relies on this function to size/fill it. On subsequent calls with the same nl, the early return leaves the new l arrays empty, so no arrows are drawn for drawvector == 4. Either remove the caching/early-return, or cache and reuse the computed l data (and invalidate it when inputs like sol/level change).

[Copilot]

vector_h/vector_hh are now derived from mesh_volume, but mesh_volume is only computed once in Init() and is not recomputed in NewMeshAndSolution(). After loading a new mesh, PrepareVectorField() will use a stale mesh_volume, which can make vector scaling incorrect. Consider recomputing mesh_volume whenever mesh is replaced (e.g., in NewMeshAndSolution() before calling PrepareVectorField()), or compute volume locally from the current mesh/bounding box here.

Suggested change

	const double volume = std::max(bb_vol, mesh_volume);
	const double volume = std::max(bb_vol, std::numeric_limits<double>::min());