Issue 687 - Add Drawings Support

[sebjf]

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.

[carmenfan]

@sebjf can you also update the config_example so I have something to reference when I ask DevOps to update the deployment please 🙈

Thank you!

yargs helper also need updating I think

[carmenfan]

@sebjf another one:

The backend is storing the rFile reference as UUID so when I run the whole pipeline it's struggling to locate the file.

Ideally we'd like to keep it as LUUID, but let me know if this is going to be super problematic. If you want to try it, ISSUE_5090 on3drepo.io is kind of hooked up (i don't know for 100% as I couldn't trigger a successful drawing process yet!) You will need to add drawing_queue config on 3drepo.io for it to work (similar to what you did on bouncer worker)

[sebjf]

Hi @carmenfan,

can you also update the config_example

Yes it would be helpful to update the one in github and not just my local! 🙈

yargs helper also need updating I think

I have done another pass and updated all the help strings too.

The backend is storing the rFile reference as UUID

DrawingRevisionNode does assume the rFile entries are UUIDs, so not sure why its not working...

(This for example is what I have been running bouncer against...)

I will check out 5090 and see if I can get it to run end-to-end!

[carmenfan]

DrawingRevisionNode does assume the rFile entries are UUIDs, so not sure why its not working.

@sebjf ah in that case it may be my fault... do let me know if the issue is on the backend 😆

[sebjf]

do let me know if the issue is on the backend 😆

OK looked into it and its not the backend! The DrawingRevisionNode assumes the rFile is a UUID... getFileRef however does not!

I will make an overload for callers who know they will be looking for UUIDs in FileManager and updating the drawings processing code to use it..

[sebjf]

Hi @carmenfan, i've added a getFileRef overload that can now pick up on the document from the backend. However I also note that FileManager::uploadFileAndCommit will create ref nodes for the svgs with _id's as strings, not UUIDs.

Would you like me to create an overload for this & have drawings use UUIDs here too?

[carmenfan]

i've added a getFileRef overload that can now pick up on the document from the backend. @sebjf Thank you 😄

Would you like me to create an overload for this & have drawings use UUIDs here too?

It would be ideal if we could (just for a better/smaller index on the database), but thanks to javascript, backend understands it either way so it's not too big of a trouble.

[sebjf]

It would be ideal if we could

Yes I think its straightforward to make an overload of uploadFileAndCommit.

I did look just now at possibly making it so all ref nodes were UUIDs by default, if the backend would be fine with some projects mixing the key types.

I think it should be possible as the only thing iirc that used non-UUID names was the previous MeshNode schema which would append strings like "_vertices"; but the new _blobRef schema no longer does that.

JavaScript would be fine, and for the few cases bouncer reads files (i.e. when loading binary mapped RepoBSON nodes), we could have getFile try one type first and fallback to the other, to emulate JavaScript.

I think if creating the overload is simple enough though, we should just do that for now, then make a maintenance ticket to change the other calling locations over to use it in the future, as this is getting out of scope now!

[carmenfan]

@sebjf up to you, I think ideally we want to convert all strings to UUIDs, but timing wise I'll leave it to your judgement

As a side note I would also like to make it so the 3D models read the file from revision node like drawings and stop getting bouncer to create a rev node (so we can start tracking individual progress instead of piling it into the model settings! - there is a ticket for this but I can't find it right now) but this is def 100% out of scope 😆

[sebjf]

@carmenfan

As a side note I would also like to make it so the 3D models read the file from revision node...there is a ticket for this but I can't find it right now

Is it this one? https://github.com/3drepo/3D-Repo-Product-Team/issues/446

(I want to add a note to whichever it is to deprecate the ref-node-id-as-string as part of it! 😆)

[carmenfan]

@carmenfan

As a side note I would also like to make it so the 3D models read the file from revision node...there is a ticket for this but I can't find it right now

Is it this one? 3drepo/3D-Repo-Product-Team#446

(I want to add a note to whichever it is to deprecate the ref-node-id-as-string as part of it! 😆)

Yes! Good job for finding it, I tried yesterday and failed miserably 🙈

[sebjf]

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!

[carmenfan]

@sebjf

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

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

[sebjf]

Thank you!

[sebjf]

@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!

[carmenfan]

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

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:

[carmenfan]

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

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:

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

[sebjf]

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!

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]

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]

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!

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]

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]

(@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]

(@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?

[sebjf]

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

[sebjf]

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]

@sebjf thanks!

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