Conversation
|
@copilot review |
|
@Soham2020sam I've opened a new pull request, #125, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Adds new Import/Export options to WebVis for downloading scenarios and exporting/importing configurations, integrating the controls into the GUI and tracking the currently loaded scenario for export.
Changes:
- Track the currently loaded scenario content/name for later download.
- Add Import/Export GUI panel: import config JSON, download current config, import scenario, download scenario, and view saved initial/final configs.
- Update
index.htmlto include hidden file inputs for scenario/config uploads.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
WebVis/src/utils.js |
Adds scenario/config download helpers and config JSON parsing for GUI-driven imports. |
WebVis/src/Scenario.js |
Stores loaded scenario content/name into shared state for export. |
WebVis/src/GUI.js |
Introduces Import/Export GUI and config-loading-from-JSON flow (replacing worker-based view). |
WebVis/index.html |
Adds hidden file inputs used by the new Import/Export GUI actions. |
.DS_Store |
Adds a macOS filesystem artifact (should not be committed). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WebVis/src/GUI.js
Outdated
| gwUser.camera.position.x = centroid.x; | ||
| gwUser.camera.position.y = centroid.y; | ||
| gwUser.camera.position.z = centroid.z + radius + 3.0; | ||
| gwUser.controls.target.set(centroid.x, centroid.y, centroid.z); | ||
|
|
||
| gwUser.miniCamera.position.x = centroid.x; | ||
| gwUser.miniCamera.position.y = centroid.y; | ||
| gwUser.miniCamera.position.z = centroid.z + radius + 3.0; | ||
| gwUser.miniControls.target.set(centroid.x, centroid.y, centroid.z); |
There was a problem hiding this comment.
loadConfigurationFromJSON updates camera/controls via gwUser, but gwUser is not defined in this ES module scope (only window.gwUser exists). This will throw a ReferenceError when importing/viewing a config. Use window.gwUser (or import gUser from main.js) consistently within this function.
| gwUser.camera.position.x = centroid.x; | |
| gwUser.camera.position.y = centroid.y; | |
| gwUser.camera.position.z = centroid.z + radius + 3.0; | |
| gwUser.controls.target.set(centroid.x, centroid.y, centroid.z); | |
| gwUser.miniCamera.position.x = centroid.x; | |
| gwUser.miniCamera.position.y = centroid.y; | |
| gwUser.miniCamera.position.z = centroid.z + radius + 3.0; | |
| gwUser.miniControls.target.set(centroid.x, centroid.y, centroid.z); | |
| window.gwUser.camera.position.x = centroid.x; | |
| window.gwUser.camera.position.y = centroid.y; | |
| window.gwUser.camera.position.z = centroid.z + radius + 3.0; | |
| window.gwUser.controls.target.set(centroid.x, centroid.y, centroid.z); | |
| window.gwUser.miniCamera.position.x = centroid.x; | |
| window.gwUser.miniCamera.position.y = centroid.y; | |
| window.gwUser.miniCamera.position.z = centroid.z + radius + 3.0; | |
| window.gwUser.miniControls.target.set(centroid.x, centroid.y, centroid.z); |
WebVis/src/GUI.js
Outdated
| // Invalidate move sequence | ||
| window.gwMoveSetSequence.invalidate(); |
There was a problem hiding this comment.
window.gwMoveSetSequence.invalidate() does not reset currentCheckpoint/currentMoveSet (see MoveSetSequence.invalidate()), so importing/viewing a config after stepping through moves can leave the overlay showing something like "Move #X / #0". Consider resetting the sequence state (e.g., reset() then invalidate, or replace window.gwMoveSetSequence with a fresh MoveSetSequence) when loading a configuration.
| // Invalidate move sequence | |
| window.gwMoveSetSequence.invalidate(); | |
| // Reset and invalidate move sequence state | |
| if (window.gwMoveSetSequence) { | |
| window.gwMoveSetSequence.reset(); | |
| window.gwMoveSetSequence.invalidate(); | |
| } |
| import { moduleBrush, pathfinderData, WorkerType, MessageType, ContentType, VisConfigData, ModuleType, getModuleAtPosition } from './utils.js'; | ||
| import { CameraType } from "./utils.js"; | ||
| import { saveConfiguration, downloadConfiguration } from './utils.js'; | ||
| import { saveConfiguration, downloadConfiguration, downloadScenario, downloadCurrentConfiguration, parseConfigurationJSON } from './utils.js'; |
There was a problem hiding this comment.
downloadConfiguration is imported here but no longer used in this file after the Import/Export refactor. Removing unused imports will avoid dead code and potential lint/build warnings.
| import { saveConfiguration, downloadConfiguration, downloadScenario, downloadCurrentConfiguration, parseConfigurationJSON } from './utils.js'; | |
| import { saveConfiguration, downloadScenario, downloadCurrentConfiguration, parseConfigurationJSON } from './utils.js'; |
| export function parseConfigurationJSON(configJSON) { | ||
| try { | ||
| const config = JSON.parse(configJSON); | ||
|
|
||
| if (!config.exists) { | ||
| console.warn("Configuration does not exist"); | ||
| return null; | ||
| } | ||
|
|
||
| // Determine module type | ||
| let moduleType; | ||
| switch (config.moduleType) { | ||
| case 'CUBE': moduleType = ModuleType.CUBE; break; | ||
| case 'RHOMBIC_DODECAHEDRON': moduleType = ModuleType.RHOMBIC_DODECAHEDRON; break; | ||
| case 'CATOM': moduleType = ModuleType.CATOM; break; | ||
| default: | ||
| console.log("Unknown module type ", config.moduleType, " -- defaulting to CUBE"); | ||
| moduleType = ModuleType.CUBE; | ||
| break; | ||
| } | ||
|
|
||
| return { | ||
| moduleType: moduleType, | ||
| modules: config.modules, | ||
| name: config.name, | ||
| description: config.description | ||
| }; |
There was a problem hiding this comment.
parseConfigurationJSON returns config.modules without validating its shape. loadConfigurationFromJSON immediately does configData.modules.forEach(...), which will throw if modules is missing or not an array. Validate that config.modules is an array (and that each entry has a position array) before returning parsed data, and return null with a clear error when invalid.
| // Update reference module to match the loaded configuration type | ||
| gReferenceModule.swapType(configData.moduleType); | ||
| gHighlightModule.swapType(configData.moduleType); | ||
|
|
There was a problem hiding this comment.
After loading a configuration, moduleBrush.type is not updated to match configData.moduleType. Since createPathfinderConfiguration() (and module placement) rely on moduleBrush.type, subsequent save/download/pathfinder runs can produce a config with the wrong moduleType/adjacency settings. Set moduleBrush.type (and update the corresponding GUI controller) when a config is loaded.
| // Ensure module brush type matches the loaded configuration's module type | |
| moduleBrush.type = configData.moduleType; |
|
|
||
| // Store the scenario content for export | ||
| pathfinderData.currentScenario = rawString; | ||
| console.log("Stored scenario content, length:", rawString ? rawString.length : 0); | ||
|
|
||
| // remove '\r' characters | ||
| rawString = rawString.replace(/\r/g, ''); |
There was a problem hiding this comment.
pathfinderData.currentScenario is captured before normalizing line endings (rawString.replace(/\r/g, '')). If a Windows-style .scen is loaded, downloading it later will re-export the un-normalized content even though the app parsed the normalized version. Consider storing the sanitized rawString (after removing \r) so export matches what was actually loaded/parsed.
| // Store the scenario content for export | |
| pathfinderData.currentScenario = rawString; | |
| console.log("Stored scenario content, length:", rawString ? rawString.length : 0); | |
| // remove '\r' characters | |
| rawString = rawString.replace(/\r/g, ''); | |
| // remove '\r' characters so parsing and export use the same normalized content | |
| rawString = (rawString || '').replace(/\r/g, ''); | |
| // Store the scenario content for export | |
| pathfinderData.currentScenario = rawString; | |
| console.log("Stored scenario content, length:", rawString.length); |
| // Reset Data | ||
| VisConfigData.nextModID = 0; | ||
| VisConfigData.clearBounds(); | ||
|
|
There was a problem hiding this comment.
Loading a configuration does not clear pathfinderData.currentScenario / currentScenarioName, so the new "Download Scenario" action can still export an older scenario after a config import/view. Consider explicitly clearing the current scenario fields when switching to a pure-configuration view to avoid exporting stale data.
| // Clear any loaded scenario, since we're switching to a pure configuration view | |
| if (pathfinderData) { | |
| pathfinderData.currentScenario = null; | |
| pathfinderData.currentScenarioName = null; | |
| } |
| import { GUI } from 'three/addons/libs/lil-gui.module.min.js'; | ||
| import { Scenario } from './Scenario.js'; | ||
| import { gScene, gLights, gRenderer, gModules, gReferenceModule, gModulePositions, gCanvas, gHighlightModule } from './main.js'; | ||
| import { gScene, gLights, gRenderer, gModules, gReferenceModule, gModulePositions, gCanvas, gHighlightModule, cancelActiveMove } from './main.js'; |
There was a problem hiding this comment.
Unused import gModulePositions.
| import { gScene, gLights, gRenderer, gModules, gReferenceModule, gModulePositions, gCanvas, gHighlightModule, cancelActiveMove } from './main.js'; | |
| import { gScene, gLights, gRenderer, gModules, gReferenceModule, gCanvas, gHighlightModule, cancelActiveMove } from './main.js'; |
| export function downloadCurrentConfiguration() { | ||
| const config = createPathfinderConfiguration(); | ||
|
|
||
| if (!config || !config.exists) { |
There was a problem hiding this comment.
This guard always evaluates to false.
| if (!config || !config.exists) { | |
| if (!config) { |
WebVis/src/GUI.js
Outdated
| reader.readAsText(file); | ||
| // Reset the input so the same file can be selected again | ||
| configUploadElement.value = ''; | ||
| } |
There was a problem hiding this comment.
Avoid automated semicolon insertion (97% of all statements in the enclosing function have an explicit semicolon).
New Import Export options