Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMU Feature #142

Merged
merged 25 commits into from
Dec 23, 2024
Merged

IMU Feature #142

merged 25 commits into from
Dec 23, 2024

Conversation

gr812b
Copy link
Collaborator

@gr812b gr812b commented Nov 19, 2024

Added a 3D model mapper, which will allow playback of IMU data

Things to add:

  • Current timestamp being played back
  • Pause / Play
  • Smarter way to play each point

You can test it at localhost:5173/IMU

@gr812b gr812b requested a review from Copilot November 19, 2024 19:18
Copy link

@Copilot 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.

Copilot reviewed 7 out of 10 changed files in this pull request and generated 4 suggestions.

Files not reviewed (3)
  • front-end/package.json: Language not supported
  • front-end/src/components/model/modelViewer.css: Language not supported
  • front-end/src/components/modal/download/DownloadModal.jsx: Evaluated as low risk
Comments skipped due to low confidence (2)

front-end/src/components/model/Eevee.jsx:7

  • [nitpick] Commented-out code should be removed to keep the code clean.
//import { Quaternion, Euler, Vector3 } from 'three';

front-end/src/lib/apiUtils.ts:14

  • The function was previously returning a Blob but now returns text. This might cause issues if the function is expected to return a Blob.
return response.text();

front-end/src/components/model/Eevee.jsx Outdated Show resolved Hide resolved
front-end/src/components/model/ModelViewer.jsx Outdated Show resolved Hide resolved
front-end/src/components/model/ModelViewer.jsx Outdated Show resolved Hide resolved
front-end/src/components/model/ModelViewer.jsx Outdated Show resolved Hide resolved
@gr812b gr812b requested a review from Copilot December 7, 2024 23:11
Copy link

@Copilot 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.

Copilot reviewed 6 out of 16 changed files in this pull request and generated 1 suggestion.

Files not reviewed (10)
  • .gitattributes: Language not supported
  • front-end/package.json: Language not supported
  • front-end/public/Eevee.obj: Language not supported
  • front-end/src/components/model/modelViewer.css: Language not supported
  • backend/src/main/java/com/mcmasterbaja/services/FileSystemStorageService.java: Evaluated as low risk
  • front-end/src/types/index.ts: Evaluated as low risk
  • backend/src/main/java/com/mcmasterbaja/services/StorageService.java: Evaluated as low risk
  • front-end/src/components/App.tsx: Evaluated as low risk
  • front-end/eslint.config.mjs: Evaluated as low risk
  • front-end/src/lib/apiUtils.ts: Evaluated as low risk
Comments skipped due to low confidence (6)

front-end/src/components/model/ModelViewer.tsx:32

  • [nitpick] Using alert for notifying the user about the replay being finished might not be the best approach. Consider using a notification component or updating the UI to provide a better user experience.
alert('Replay finished!' + event);

front-end/src/components/model/ModelViewer.tsx:4

  • Since this is a TypeScript file, it should ideally be imported from ./Eevee.tsx.
import { Eevee } from './Eevee.js';

front-end/src/lib/modelUtils.ts:5

  • [nitpick] The function name extractColumn could be more descriptive. Consider renaming it to extractColumnData for better clarity.
const extractColumn = (data: string[][], columnIndex = 1) => {

front-end/src/lib/modelUtils.ts:137

  • [nitpick] The warning message in setSpeed could be more informative. Consider changing it to console.warn('Speed must be a positive number. Ignoring invalid value:', newSpeed);.
console.warn('Speed must be positive. Ignoring invalid value:', newSpeed);

backend/src/main/java/com/mcmasterbaja/FileFetchResource.java:50

  • Ensure that the FileNotFoundException provides a clear and helpful error message.
throw new FileNotFoundException("File not found: " + filekey);

backend/src/main/java/com/mcmasterbaja/FileFetchResource.java:50

  • Verify that the FileNotFoundException is defined correctly and extends the appropriate exception class.
throw new FileNotFoundException("File not found: " + filekey);

Copy link
Contributor

@camdnnn camdnnn left a comment

Choose a reason for hiding this comment

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

Seems pretty good to me just one small pathing issue that should be updated

@gr812b gr812b requested a review from Copilot December 22, 2024 05:01

Choose a reason for hiding this comment

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

Copilot reviewed 6 out of 16 changed files in this pull request and generated no comments.

Files not reviewed (10)
  • .gitattributes: Language not supported
  • front-end/package.json: Language not supported
  • front-end/public/Eevee.obj: Language not supported
  • front-end/src/components/model/modelViewer.css: Language not supported
  • front-end/eslint.config.mjs: Evaluated as low risk
  • front-end/src/components/App.tsx: Evaluated as low risk
  • front-end/src/components/model/Eevee.tsx: Evaluated as low risk
  • front-end/src/components/model/ModelViewer.tsx: Evaluated as low risk
  • front-end/src/lib/apiUtils.ts: Evaluated as low risk
  • front-end/src/lib/modelUtils.ts: Evaluated as low risk
Copy link
Contributor

@camdnnn camdnnn left a comment

Choose a reason for hiding this comment

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

LGTM!

@gr812b gr812b merged commit 1816718 into main Dec 23, 2024
3 checks passed
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.

2 participants