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

DUI3-325 Add Civil3d 2024 connector #3531

Merged
merged 14 commits into from
Jun 25, 2024
Merged

Conversation

clairekuang
Copy link
Member

Description & motivation

Adds the Civil3d 2024 connector, with just a send binding, to the Autocad connector folder. Also adds a corresponding Autocad2024 converter so that civil3d connector can send out of the box.

Changes:

  • Noteworthy changes are a separate Civil3dConnectorModule in the connector to allow for Civil3d bindings to differ from Autocad, and preprocessor directives in AutocadCommand to create an AutocadSettings with the correct host app and version

Screenshots:

Sent a point from Civil3d: https://app.speckle.systems/projects/b53a53697a/models/719612f9da

@oguzhankoral oguzhankoral self-assigned this Jun 25, 2024
Copy link
Member

@oguzhankoral oguzhankoral left a comment

Choose a reason for hiding this comment

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

Overall looks good and good job!
Few comments maybe small changes needed.

@@ -64,6 +64,7 @@ public static class HostApplications
Unity = new("Unity", "unity"),
GSA = new("GSA", "gsa"),
Civil = new("Civil 3D", "civil3d"),
Copy link
Member

Choose a reason for hiding this comment

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

do we need this one if we added Civil3D?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, to prevent breaking the DUI2 connectors

Copy link
Member

Choose a reason for hiding this comment

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

I believe we should start to consider have new Applications(?) class for DUI3 only instead populating old.

Copy link
Member

Choose a reason for hiding this comment

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

FYI: @BovineOx added as subtask under the other relevant story (assigned to Ian) https://spockle.atlassian.net/browse/DUI3-431

@@ -124,6 +125,11 @@ public static HostApplication GetHostAppFromString(string? appname)
return Civil;
}

if (appname.Contains("civil3d"))
Copy link
Member

Choose a reason for hiding this comment

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

what is the difference with "civil"? because this will always return the previous one I believe because it will contain already "civil"

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah good point, will fix this

@@ -1,3 +1,4 @@
#if AUTOCAD
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense to put note here why we do this

Copy link
Member

Choose a reason for hiding this comment

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

We can have shared module for autocad verticals, seems we are having many dups

// Register bindings
builder.AddSingleton<IBinding, TestBinding>();
builder.AddSingleton<IBinding, AccountBinding>();
builder.AddSingleton<IBinding, AutocadBasicConnectorBinding>();
Copy link
Member

Choose a reason for hiding this comment

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

I believe this is unnecessary registration since we register it on next line as IBasicConnectorBinding

Copy link
Member Author

Choose a reason for hiding this comment

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

This was preexisting, so I'm leaving it here for now

Copy link
Member

Choose a reason for hiding this comment

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

fair enough

@clairekuang clairekuang merged commit 85c2ff6 into dui3/alpha Jun 25, 2024
33 checks passed
@clairekuang clairekuang deleted the DUI3-26-DUI3-Civil3D branch June 25, 2024 13:04
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