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

Issue 687 - Add Drawings Support #688

Merged
merged 40 commits into from
Aug 16, 2024
Merged

Issue 687 - Add Drawings Support #688

merged 40 commits into from
Aug 16, 2024

Conversation

sebjf
Copy link
Contributor

@sebjf sebjf commented Jul 1, 2024

This fixes #687

Description

Adds support for processing drawings in a similar manner to 3d models, but where the drawing to process is provided via an existing revision node.

Specifically, this PR:

  1. Makes RevisionNode a superclass and ModelRevisionNode & DrawingRevisionNode subclasses, with members appropriately scoped to each.
  2. Adds the ability to get a path from an fs ref node, since that is how files will be provided for drawings as opposed to via the bouncer import config
  3. Extends the FileProcessor superclass so that it can now be initialised to collect geometry data, SVG data, or both, from a single file (SVG data collected via the DrawingImageInfo type, which is analogous but not directly equivalent to GeometryCollector).
  4. Adds the ability to import to SVGs, via DrawingImageInfo, to the DWG and DGN file processor subclasses. This uses the SvgExportModule which is included as part of ODA.
  5. Introduces a new class DrawingManager, which is responsible for interacting with the database
  6. Introduces a new class DrawingImportManager, which is responsible for selecting the right importer and using it to populate the DrawingImageInfo
  7. Adds a new method, processDrawingRevision, to RepoManipulator & RepoController, that will acquire a file from a drawing revision, process it using the aforementioned importers, and update the revision node in the database.
  8. Hooked up processDrawingRevision to a new command processDrawing in 3drepobouncerClient
  9. Added unit tests for the new functionality

Additionally, changes to bouncer_worker include:

  1. A new queue handler, drawingQueueHandler.js, has been added, as well as a new label to queueLabel for it.
  2. Queue handler definitions have been decoupled from the config, with the associations now being made in queueHandler.js
  3. queueHandler.js has been refactored slightly so that dereferencing (and validation) of labels to queue names from the config happens immediately in connectToQueue or runNTasks, and from there on in the queue names are used to resolve handlers, allowing queue initialisation and handling methods to be more generalized throughout.

sebjf added 21 commits June 18, 2024 10:05
…d update) and removed -f from processDrawing args
…s from import manager when compiling without oda support
…specifics from import manager when compiling without oda support"

This reverts commit 4fc28e5.
…dler def and initialisation of multiple queues more general
@sebjf sebjf requested a review from carmenfan July 1, 2024 12:14
@carmenfan carmenfan self-assigned this Jul 16, 2024
tools/bouncer_worker/src/lib/queueHandler.js Outdated Show resolved Hide resolved
tools/bouncer_worker/src/lib/queueHandler.js Outdated Show resolved Hide resolved
tools/bouncer_worker/src/queues/modelQueueHandler.js Outdated Show resolved Hide resolved
tools/bouncer_worker/src/lib/queueHandler.js Outdated Show resolved Hide resolved
@sebjf
Copy link
Contributor Author

sebjf commented Aug 7, 2024

Hi @carmenfan, did you say that size and format had been added to the queue message?

It doesn't appear to be so in 5090, though I am probably looking in the wrong place!

image

@carmenfan
Copy link
Member

carmenfan commented Aug 7, 2024

@sebjf

It should be added as part of the queueMeta, which is the data object in the function you're looking at
image

And eventually gets written inside importParams.json (equivalent to <corId>.json file in the 3D Model implementation)
image

@sebjf
Copy link
Contributor Author

sebjf commented Aug 7, 2024

Thank you!

@sebjf
Copy link
Contributor Author

sebjf commented Aug 7, 2024

@carmenfan,

This is now receiving a post request like so via 5090,

https://3drepo.lan/api/v5/teamspaces/sebjf/projects/59284d10-4b00-11ed-ac46-93641d60e710/drawings/17e4db45-bf25-4b87-a5ad-00a2e781217f/revisions?key=04befbd4f356b2117b6a7ddc344e6f97

And processing it successfully.

We might want to wait for the endpoint to get the SVGs to make sure it can be read as well, but if that works all the changes should be addressed now!

Copy link
Member

@carmenfan carmenfan left a comment

Choose a reason for hiding this comment

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

@sebjf There's a type descrepancy on the id of the image:

tjhe entry in .history is written as a UUID:
image

But the ID in .ref is in string
image

JS will just pass it as is to the database query so mongo will return with no entry. So we either need both to be string or both to be UUID 😢

Also, can we conver the extension on the name to be .svg please?

@carmenfan
Copy link
Member

carmenfan commented Aug 11, 2024

@sebjf another thing, just checking out the svg against the same drawing exported to PDF, this is what the PDF looks like:
image

And this is the DWG. The annotations are coming out in weird colours, sometimes it's it's obscured as well because the background colour is the same as the font colour:
image

@carmenfan
Copy link
Member

@sebjf another thing, just checking out the svg against the same drawing exported to PDF, this is what the PDF looks like: image

And this is the DWG. The annotations are coming out in weird colours, sometimes it's it's obscured as well because the background colour is the same as the font colour: image

Opening it the ODA app i can see it's assuming a black background... I wonder if this is a setting we can tweak...
image

@sebjf
Copy link
Contributor Author

sebjf commented Aug 13, 2024

Hi @carmenfan,

@sebjf There's a type descrepancy on the id of the image:

Gah, missed it after the refactor - fixed!

Also, can we conver the extension on the name to be .svg please?

Done!

image

I noticed though that model in the revision node created by .io is a string too. Is this intentional? (bouncer reads it and assumes its a UUID so I'll need to update it if so)

@sebjf
Copy link
Contributor Author

sebjf commented Aug 13, 2024

Hi @carmenfan,

Opening it the ODA app i can see it's assuming a black background... I wonder if this is a setting we can tweak...

Yes, the OdGiContextForDbDatabase has a few settings. I have updated the palette background and that seems to have worked. I also updated the minimum line with and it looks a little closer to Revit too.

@carmenfan
Copy link
Member

Hi @carmenfan,

@sebjf There's a type descrepancy on the id of the image:

Gah, missed it after the refactor - fixed!

Also, can we conver the extension on the name to be .svg please?

Done!

image

I noticed though that model in the revision node created by .io is a string too. Is this intentional? (bouncer reads it and assumes its a UUID so I'll need to update it if so)

model is stored as a string because of legacy reason (if you open the settings collection in a db you'll see they're all string ID, and it'll be difficult to store it as LUUID in one place and string in other as they're usually passed around and queried everywhere)

I do want to convert to UUID in the future but it's going to take a big migration script 🤦... so future work 😆

@sebjf
Copy link
Contributor Author

sebjf commented Aug 13, 2024

I do want to convert to UUID in the future but it's going to take a big migration script 🤦... so future work 😆

Got it - bouncer has been updated to consider it a string!

@sebjf
Copy link
Contributor Author

sebjf commented Aug 13, 2024

(@carmenfan , something I just remembered was that ODA fails to set the width and height tags of its SVGs, which Chromium at least needs to render anything sensible, so we should leave this open until that's added at least - we'll probably need to add those ourselves to the string before its saved.)

@carmenfan
Copy link
Member

(@carmenfan , something I just remembered was that ODA fails to set the width and height tags of its SVGs, which Chromium at least needs to render anything sensible, so we should leave this open until that's added at least - we'll probably need to add those ourselves to the string before its saved.)

@sebjf not 100% sure what this means 😆 . Are we waiting on a fix from ODA? or do we need add this ourselves as part of this branch?

carmenfan
carmenfan previously approved these changes Aug 14, 2024
Copy link
Member

@carmenfan carmenfan left a comment

Choose a reason for hiding this comment

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

LGTM - just need to double check with you what the SVG height/width entails 😆

@sebjf
Copy link
Contributor Author

sebjf commented Aug 14, 2024

@sebjf not 100% sure what this means 😆 . Are we waiting on a fix from ODA? or do we need add this ourselves as part of this branch?

Sorry end of the day post 😆

SVG files have both a viewBox, and width and height tags. I am not sure why they need all three since viewBox should do the job. But for whatever reason, they do. ODA however doesn't write the width and height properties.

This should not be problem in theory because the viewBox is known, and the true with and height of the renderer is set by the DOM. However, in practice odd things happen if they aren't present. For example, it seems Firefox assumes the fields are present and there are many bugs like this from downstream libraries that use SVGs with it.

At first I was just going to make sure our 2D viewer could handle SVGs without the tags, but given Firefox's issues, I think the best solution is just to add the fields on export. We know the width and height because we set up the view properties ourselves.

This is something we'd do in bouncer; we wouldn't try and get ODA to modify their exporter.

I will do it today as part of this ticket, in case we want to merge before Milestone 5.

…, and added unit test to make sure they dont break in the future
@sebjf
Copy link
Contributor Author

sebjf commented Aug 14, 2024

Hi @carmenfan,

Added the attributes. There are a couple more magic numbers/strings than I usually like but not sure its worth creating an svg helper header yet - I've updated the unit tests in case the svg exporter changes with the ODA upgrade.
(I guess we could put a function in the FileProcessor base class, let me know if thats preferred!)

@carmenfan
Copy link
Member

@sebjf thanks!

Probably prefer it to be in the base class but not really a big issue 😆

@carmenfan carmenfan merged commit 7d88af6 into staging Aug 16, 2024
3 checks passed
@carmenfan carmenfan deleted the ISSUE_687 branch August 16, 2024 12:37
@carmenfan carmenfan removed their assignment Aug 20, 2024
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.

3 participants