specklesystems / speckle-sharp

.NET SDK, Schema and Connectors: Revit, Rhino, Grasshopper, Dynamo, ETABS, AutoCAD, Civil3D & more.
https://speckle.systems
Apache License 2.0
354 stars 166 forks source link

Dim/dui3/send caching di (DUI3-79) #3491

Closed didimitrie closed 1 month ago

didimitrie commented 1 month ago

DI-ifies the send conversion cache, and removes a bunch of wiggle code that was no longer needed as a result.

Points of focus for the review is arcgis: it builds, but i haven't tested it locally to see that it does what it's supposed to. Do note the last paragraph on the cache class:

Caching is optional in the send ops; an instance of this should be injected only in applications where we know we can rely on change tracking!

If we do not trust change detection in arcgis (ie, difficult re object selection, layers, etc.), it can be skipped entirely!

https://spockle.atlassian.net/browse/DUI3-29

didimitrie commented 1 month ago

My main question on having caching nullable, how it can be null?

Basically I am not sure if all connectors will have this cache, so hence it's nullable (optinal) everywhere. Send should work without it - for example, if i comment out in rhino this line,

    // register send conversion cache
    builder.AddSingleton<ISendConversionCache, SendConversionCache>();

in the DUI3-DX/Connectors/Rhino/Speckle.Connectors.Rhino7/DependencyInjection/RhinoConnectorModule.cs, it will send as always, but just not use the cache.

Does this make sense @oguzhankoral?

I think I get it now (after i replied here https://github.com/specklesystems/speckle-sharp/pull/3491#discussion_r1631180249) : pretty much the only place it actually needs to be nullable is in the shared RootObjectSender class, which is shared by any connector, including ones that will potentially not have this cache.

That okay ?

KatKatKateryna commented 1 month ago

As a general comment, the app is running and changedObjectIds are detected as before. Re ArcGIS change detection: ATM, we track it on layer level, and it works well. More granularity (and more complexity) can be added for report purpose, but for Selecting/Updating selection looks fine to me

BovineOx commented 1 month ago

My main question on having caching nullable, how it can be null?

Basically I am not sure if all connectors will have this cache, so hence it's nullable (optinal) everywhere. Send should work without it - for example, if i comment out in rhino this line,

    // register send conversion cache
    builder.AddSingleton<ISendConversionCache, SendConversionCache>();

in the DUI3-DX/Connectors/Rhino/Speckle.Connectors.Rhino7/DependencyInjection/RhinoConnectorModule.cs, it will send as always, but just not use the cache.

Does this make sense @oguzhankoral?

I think I get it now (after i replied here #3491 (comment)) : pretty much the only place it actually needs to be nullable is in the shared RootObjectSender class, which is shared by any connector, including ones that will potentially not have this cache.

That okay ?

As I mentioned above we should probably have a NoCacheSendConversionCache() I am unsure if the Singleton() is the right scope also, maybe, probably it should be as the cache will persist across sends.

didimitrie commented 1 month ago

Oki doki, ready for review again:

  • made the cache non-optional anywhere, and added a do nothing one for future use in cnx's that can't support it (cc @BovineOx @oguzhankoral )
  • removed stale class and comments (cc @JR-Morgan @oguzhankoral)

Also thx @KatKatKateryna for testing arcgis!