-
Notifications
You must be signed in to change notification settings - Fork 8
Issue 165 #169
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
base: main
Are you sure you want to change the base?
Conversation
|
@UcheWendy I didn't change 111 files, it shows that because I had to add the data folder to get the annotations working. These are the files I modified: Files Changed for Issue #165
|
UcheWendy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code you wrote for the feature itself is very high-quality.
Excellent Architecture: Creating a new, self-contained js/annotationOverlay.js module is the perfect choice. It’s small, has a clear purpose, and doesn't overcomplicate other files.
Correctly Solved the Hardest Problem: You correctly identified that this is a coordinate math problem. Your solution is excellent:
You defined the PowerPoint EMU constants (PPT_EMU).
Your emuRectToPx and emuPointToPx helper functions are the correct way to handle the scaling.
It creates an SVG and a separate div for labels (.annotation-stage).
Great CSS: The new styles in style.css are perfect. You've correctly used position: absolute and z-index to create the overlay.
Clean Integration: You've correctly modified imageDisplay.js and dropdowns.js to call loadAndDrawAnnotations and clearAnnotations at the right times.
🚨 Critical Issues: PR Scope & Architecture
This PR cannot be merged in its current state. The logical changes are good, but they are bundled with thousands of unrelated files that break the project's architecture.
Massive Scope Creep:
The Problem: This PR adds over 2,000 files that are unrelated to the issue. It includes new GitHub templates, a LICENSE, CODEOWNERS, and the entire data structure for the whole project (images, descriptions, colored regions, etc.) in a new templates/data directory.
The Fix: A pull request must only contain the code needed to solve its specific issue. All of these extra files must be removed.
Major Architectural Violation (New Data Source):
The Problem: You added a new templates/data directory. The established architecture of this project is that the backend (boneset-api) fetches all data from the GitHub data branch. The frontend (templates) should never contain its own data folder. This PR creates a second, conflicting source of truth.
The Fix: This entire templates/data directory and all its contents must be deleted from this PR.
Architectural Violation (Hardcoded URL):
The Problem: Your dropdowns.js file has a hardcoded path to a local file: ./templates/data/DataPelvis/annotations/text_label_annotations/slide02_bony_pelvis.json. This breaks the architecture and will not work on a deployed server. The frontend must not read from its own filesystem.
The Fix: The frontend must ask the backend for this data. You need to create a new API endpoint in server.js (e.g., /api/annotations/:boneId). The frontend will call this endpoint, and the backend will be responsible for fetching the correct JSON file from the GitHub data branch and returning it.
💡 How to Fix This PR (Required Action)
This is a very common issue, and the fix is straightforward:
Do not modify this PR.
Create a new, clean branch from main.
Copy only the 5 files that are part of this feature into your new branch:
templates/boneset.html (just the 1-line script addition)
templates/js/annotationOverlay.js (your new file)
templates/js/dropdowns.js (your changes)
templates/js/imageDisplay.js (your changes)
templates/style.css (the new annotation styles)
Fix the hardcoded URL: In your new branch, modify dropdowns.js to call a (new) backend API endpoint instead of a local file.
Submit a new, clean PR with just these 5 files.
Added text label annotations for the Bony Pelvis slide (slide02_bony_pelvis.json).
Implemented logic in annotationOverlay.js and dropdowns.js to only display annotations when the selected boneset is “Bony Pelvis.”
Ensured annotations disappear when switching to other bonesets.
Adjusted the EMU-to-pixel mapping and positioning logic to align the labels with the correct anatomical areas.
Updated styling to improve readability and layering of annotations on top of bone images.
Previously, the Bony Pelvis slides didn’t have visible annotations, making it difficult for users to identify key features like the Ilium, Pubis, and Ischium.
This update helps users clearly see anatomical relationships and interact with the slide more effectively.
Added a new annotation JSON file (slide02_bony_pelvis.json) with label and line coordinates.
Integrated a crop template (template_bony_pelvis.json) to align slide coordinates with displayed images.
Updated JS logic to fetch, parse, and render labels dynamically when “Bony Pelvis” is selected.
Fine-tuned coordinate mapping to ensure annotations track correctly with image positioning.
The annotations are now working, but there’s a slight positional offset on some screens due to scaling differences between PowerPoint EMU coordinates and displayed image ratios.
Currently testing small adjustments to the crop normalization to achieve a perfect alignment.
Will request additional review or suggestions for fine-tuning overlay scaling.
Before: No annotations shown for Bony Pelvis slides.
After: Text label annotations appear correctly for Bony Pelvis; hidden for other selections.