Filing patch#45
Conversation
There was a problem hiding this comment.
@Crusific Shoot me a text if you need some more clarity on any of my comments, I'd be happy to hop on a call as well. We can even keep the conversation going within this PR if you wish to resolve the comments.
@kaiden-keane assist me with this review, I think I've addressed all my concerns but I'd like some feedback on whether you agree with my comments and/or if you have any of your own.
There was a problem hiding this comment.
These changes do not belong in this PR. This seems like a refactoring change to me. Separate PR must be created to address these issues.
There was a problem hiding this comment.
This is more than the PR comment describes.
| function UAVStatusComponent({ label = "", value }) { | ||
| function UAVStatus({ status }) { | ||
| return ( | ||
| <div className="flex justify-between items-center space-x-2"> | ||
| <label className="basis-1/2">{label}</label> | ||
| <div className="basis-1/2 rounded-md border px-2 py-2 font-mono text-sm"> | ||
| {value} | ||
| </div> | ||
| </div> | ||
| <Card className="w-full h-full"> | ||
| <CardHeader className="pb-3"> | ||
| <CardTitle className="text-sm">UAV</CardTitle> | ||
| </CardHeader> | ||
| <CardContent className="space-y-2"> | ||
| <div className="flex justify-between text-xs"> | ||
| <span>Connected</span> | ||
| <span className="font-mono">{status.connection}</span> | ||
| </div> | ||
| <div className="flex justify-between text-xs"> | ||
| <span>Mode</span> | ||
| <span className="font-mono">{status.mode}</span> | ||
| </div> | ||
| <div className="flex justify-between text-xs"> | ||
| <span>Images</span> | ||
| <span className="font-mono">{status.imageCount}</span> | ||
| </div> | ||
| <div className="flex justify-between text-xs"> | ||
| <span>Time</span> | ||
| <span className="font-mono">{status.timeSinceMessage}s</span> | ||
| </div> | ||
| </CardContent> | ||
| </Card> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why change this function at all? This PR has no concern with it. If refactoring, should go in another PR
| // Save depth image | ||
| const depthPath = path.join(imageDir, "depth.png"); | ||
| if (depthBuffer) { | ||
| fs.writeFileSync(depthPath, depthBuffer); | ||
| } | ||
|
|
There was a problem hiding this comment.
Don't save depth map, the frontend doesn't need it. We only need coordinates of targets. Sending depth map from drone and saving it uses a lot of bandwidth that we can skip on.
| <FilingSystemLayout | ||
| selectedImage={selectedImage} | ||
| onSelectImage={setSelectedImage} | ||
| /> |
There was a problem hiding this comment.
Confusing design... FilingSystemLayout is responsible for running onSelectImage when an image is selected inside it, but it also needs to know which image is currently selected?
Why not have a state variable inside FilingSystemLayout that keeps track of the selected image if it is responsible for selecting the images?
| <> | ||
| <Card className="w-full h-full shadow-2xl"> | ||
| <CardHeader> | ||
| <CardTitle>UAV</CardTitle> | ||
| </CardHeader> | ||
| <CardContent className="space-y-2"> | ||
| <UAVStatusComponent label="Connected" value={status.connection} /> | ||
| <UAVStatusComponent label="Time since last message" value={`${status.timeSinceMessage} sec`} /> | ||
| <UAVStatusComponent label="Current mode" value={status.mode} /> | ||
| <UAVStatusComponent label="Pictures received" value={status.imageCount} /> | ||
| </CardContent> | ||
| </Card> | ||
| </> |
There was a problem hiding this comment.
Better to keep this current layout of UAVStatus, unless you have a better reason to change it.
There was a problem hiding this comment.
Read comment on DOCUMENTATION_INDEX.md
There was a problem hiding this comment.
Read comment on DOCUMENTATION_INDEX.md
There was a problem hiding this comment.
Read comment on DOCUMENTATION_INDEX.md
There was a problem hiding this comment.
Read comment on DOCUMENTATION_INDEX.md
There was a problem hiding this comment.
If re-worded, this can be the PR description rather than a file of its own
| app.get("/api/imageTargets/:id1/:id2", (req, res) => { | ||
| const id = `${req.params.id1}/${req.params.id2}`; | ||
| const targets = getImageTargets(id); | ||
| app.get("/api/imageTargets/:imageId", (req, res) => { | ||
| const imageId = req.params.imageId; | ||
| const targets = getImageTargets(imageId); |
There was a problem hiding this comment.
This change should be undone, /:id1/:id2 is important because this is the nameing convention shepard follows
There was a problem hiding this comment.
Refactoring changes... This is actually some pretty good refactoring! great work, but this belongs in another PR
3 participants
This PR introduces a complete RGBD filing system for the Emu UAV ground station software, enabling professional storage, browsing, and measurement of OAKD camera images with integrated distance capabilities. The implementation transforms the existing layout into a 4-section interface optimized for UAV operations.