speckleworks / specklecore Goto Github PK

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

Clients, when deserialised, should use the current user's api token.

Actual Behaviour

They use the creator's api token (they get serialised with it).

Affected Projects

Will propagate through Speckle Rhino and co, but ideally on the dev side changes will be minimal.

Server returns a list of objectIds, response thinks its called Objects.

The Rhino and Grasshopper clients have various update methods that have been tailored to their environment. TODOs:

• Bring those back into core, as they can be generally more useful
• Update/progress event mechanisms should also be brought back into core

This is a big, non-urgent, issue

I did some more work on the DSConverter and this is what is still open and maybe SpeckleCore issues?
I only did a quick test, it is not checked if geometry perfectly matches input 100%, yet.

Nurbs issues

ERROR
Unable to create BSplineCurve from vertices : B-spline knot sequence is decreasing (should be non-decreasing)

NOTE
Degree: Should be greater than 1 (piecewise-linear spline) and less than 26 (the maximum B-spline basis degree supported by ASM). Weights: All weight values (if supplied) should be strictly positive. Weights smaller than 1e-11 will be rejected and the function will fail. Knots: The knot vector should be a non-decreasing sequence. Interior knot multiplicity should be no larger than degree + 1 at the start/end knot and degree at an internal knot (this allows curves with G1 discontinuities to be represented). Note that non-clamped knot vectors are supported, but will be converted to clamped ones, with the corresponding changes applied to the control point/weight data. Knot Array: The array size must be num_control_points + degree + 1

What do we change in SpeckleCore, which needs to be adjusted in other places?

Essentially, he freaks out and does not serialise them.

Whatever works, and we get rid of the mess I made.

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

Clients should set accept-encoding: gzip headers and properly decompress responses.

Actual Behaviour

Server returns uncompressed data. 🤕

Affected Projects

All projects dependent on SpeckleCore should update once this is fixed.

Speckle Core converter should be the single point of contact for conversions, and the application specific converters should just add extension methods to their types, for which the core converter will look into.

Flow as it is now:

Object -> AppConverter (ie, rhinoconverter) -> checks type & converts | tries to convert to abstract -> converted object

Flow as it can be:

Object -> SpeckleCore.Converter.Convert -> checks for extension method "ToSpeckle" -> if yes, returns the result of that, if not converts to abstract

Simpler clearer and less switches! Or just switching problems around? 🤔

Nulls are currently not supported by Core and the various converters:

Proposed Solution (if any)

Add a SpeckleNull type.

Add project number please - this is often used together with the name.

Step 0:

• I've read the contribution guidelines!

Implement speckleworks/SpeckleSpecs#25

This should not affect deployment usability like #7 so it's a go

Where x = 10mb deflated, or x is grabbed from the root api endpoint. These errors should nicely bubble up into clients.

For example, the rhino radial dimension has public properties/fields, that nevertheless can not be set due to the restrictions of rhino, throwing an ominous DocumentCollectedException: This object cannot be modified because it is controlled by a document.

Fix (in progress): wrap all property/field setters in try { } catch { } and take care on implications when it comes to references (some might be null!).

Remove api calls for stream layers & stream name
They should be replaced with stream patch

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

Getting all streams for a user should be GET <server>/streams.

Actual Behaviour

This fails with a 400 response ("Something went wrong.").
Adding ?omit=objects makes it work. Something goes wrong when it tries to include the objects in the stream list.

Affected Projects

Core, server.

Reproduction Steps & System Config (win, osx, web, etc.)

Proposed Solution (if any)

For now, adding ?omit=objects makes it work.

Step 0:

• I've read the contribution guidelines!

We probably need an interface for the interop class that applications using SpeckleView can rely on:

• ShowDev
• SaveFileClients
• GetFileClients
• GetUserAccounts
• AddUserAccount
• RemoveUserAccount
• ...

More things to come...

Expected Behaviour

Start point on a circle domain should be based on the plane the curve has been defined in

Actual Behaviour

When circles are created on twisted frames, before translation evalutation correctly picks up curve start, afterwards, the start point defaults to standard coordinates.

byObject, byLayer?

For streams, objects, clients, users, comments

This would allow for objects to be deserialised to native types even if the assembly name differs, as the case might be when producing libraries for various applications that don't share a core lib

The number of points does not tell you if the last point is supposed to be connected with the first one. Add boolean parameter IsClosed or something similar?

Concerning the future SpeckleRhino interface, I noticed that there is no code written to handle ToNurbsCurve() conversions (in order to retrieve the UserDictionary), like it is done for the Grasshopper component here:
https://github.com/speckleworks/SpeckleUserDataUtils/blob/dev/SetUserDataComponent.cs#L67-L74

Therefore I propose to add the same conversion here (I am not sure if this is consistent):

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

SpeckleAbstract hash should be unique based on its property, type, and assembly.

Actual Behaviour

Hash is only based on property dictionary. This causes issues when different objects with the same property dictionary are sent as it causes collisions.

Affected Projects

SpeckleCore

Reproduction Steps & System Config (win, osx, web, etc.)

1. Create two classes with the same properties.
2. Populate objects with same values.
3. Allow SpeckleCore to serialize and send objects.

Proposed Solution (if any)

Add the assembly and type to the hashes of SpeckleAbstracts.
Patch

I noticed that the Add User component exposes full email addresses.

I know this is nasty stuff, but given that users don't have to accept any T&C to use the service and that a new regulation on data protection will soon take effect in the EU, I think we should be careful with user data.

To make things snappier and nice fallback for offline. Would be nice to have a 'make available offline' sort of functionality.

Some notes here about what I was suggesting for StreamAdd.

StreamAdd would be useful in the case scenario where the user adds to an existing stream (containing objects & layers) new objects and new layers (among those, some objects or some layers might already exist in the stream).

The following steps would be:
1 - running through all the objects the user intends to add and dismiss the ones whose Guid matches with existing ones in the DB.
2 - running through all the layers the user intends to add and dismiss the ones whose Guid matches with existing ones in the DB.
3 - Finally, it remains to the user to organize his objects and layers so they can match startIndex and objectCount before calling StreamAdd. However, StreamAdd should be aware of the number of objects existing in the stream, so the startIndices found by the user when running through the new list of objects he intends to add (let's say 0-2, 3-8, 9-13) would be shifted and become (if 8 objects exist): 8-11, 12-17 etc. etc.
Let's hope that this way of operating would not be too expensive with a large amount of objects

The SpeckleAbstract Type has a few cool ideas in it, but after initial frolicking, we will defer to several sets of good practice, namely:

• Serialise classes that are marked as [Serializable]
• Respect properties/fields that are marked with [NotSerialised]
• Enforce safe dynamic instantiation based on empty constructors

Also docs will need to be updated...

To keep in mind, also custom attributes could be used, on the lines of [SpeckleSafe] to mark safe-for-speckle classes, and [SpeckleSkip] to mark fields/properties that should not be serialised.

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

Deserializing a SpeckleAbstract object which has a field or property of a type List<int> properly populates the property.

Actual Behaviour

Currently returns the object with an empty list. Behavior is due to the fact that all numbers are received as type long which results in an error when adding to a List<int>.

Affected Projects

SpeckleCore

Reproduction Steps & System Config (win, osx, web, etc.)

1. Create class with a property of List<int>.
2. Set the value of the property to be non-empty list.
3. Allow SpeckleCore to serialize and send the object.
4. Receiving the object returns an object with an empty list.

Proposed Solution (if any)

Convert the object to be added to the property/field list explicitly.
Patch

Figure out AppVeyor NuGet plumbing.

Because we're no longer checking for the assembly in ToAbstract and only in Serialise, objects that have ToSpeckle methods do not get those called.

Essentially Serialise and ToAbstract should be combined with the initial checks from Serialise. TODO: check the deserialisation routes

ref speckleworks/SpeckleGrasshopper#7
migrating issue, filed in a bad repo

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

SpeckleObject should have a Duplicate() method.

Actual Behaviour

It doesn't.

Affected Projects

Projects where you want to duplicate a SpeckleObject.

Reproduction Steps & System Config (win, osx, web, etc.)

Try to use SpeckleObject.Duplicate()

Proposed Solution (if any)

Implement Duplicate() for SpeckleObjects.

Should probably throw an exception when no internet is available or the API cannot be reached. If the call below fails there is not catch to handle it:

Things like Speckle.RhinoConverter and its Speckle.RevitConverter cousin should implement the same ISpeckleConverter interface. This came out of issue 4 on SpeckleRevit repo where I realised there wasn't a base class or interface defined for what these converters should implement as a minimum.

Same goes for the clients themselves, for example the Rhino client defines an interface for itself in the client repo (filename doesn't match contents btw @didimitrie @fraguada ) but it doesn't look like Speckle.Core defines or has any opinions on what a client should implement.

Provisional list of interfaces required :

• ISpeckleConverter

• ISpeckleClient

• ISpeckleSender & ISpeckleGetter ?

I might be off-base here, keen to hear what others think ?

Step 0:

• I've read the contribution guidelines!

Proposed Spec Change

[ ] Introduce "Name" as a standard property on SpeckleObject
[ ] Rename existing property "Name" on SpeckleBlock to "BlockName"

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

Users should be able to register.

Actual Behaviour

Registration throws on error from the SpecklePopup.

Affected Projects

SpeckleGrasshopper, SpeckleDynamo

Step 0:

• I've read the contribution guidelines!

Expected Behaviour

Speckle objects should have a transform property as defined by the specs

Actual Behaviour

They don't.

Affected Projects

Contained to speckle core as it's not used yet throughout.

Reproduction Steps & System Config (win, osx, web, etc.)

https://github.com/speckleworks/SpeckleCore/blob/master/SpeckleCore/ModelObjects.cs#L122-L151

Might be problematic for offset curves...

This is a tricky one:
On the server side, compression should be enabled on the webserver/proxy level (nginx). As such, if implementations details vary - someone will not go through the extra steps of enabling compression, or will not have a proxy - then the client will fail.

Can we please get back SpeckleLine class instead of using SpecklePolyline with 2 points?
Thankssss

Step 0:

• I've read the contribution guidelines!

Issue

The api for converting between speckle and native types currently hides the dependency on extension methods named ToNative and ToSpeckle that lie in an assembly with Speckle and Converter in its name. Reading the code or documentation is the only way to realise the requirement of this dependency. This doesn’t offer a lot of flexibility in choosing how converters are consumed by the Speckle api.

Impact

Any projects where the current converter is used.

Looking through the repo, this effects:

• SpeckleDynamo
• SpeckleUnity
• SpeckleRhino

Solution

Just an idea on how to solve this:

• Converters to be injected as a dependency into a core converter class. This class will then handle conversion between types using the converters. This offers a layer of abstraction between the implementations that govern the conversion between speckle and native types and the calling code, without hiding the dependency on converters (namely ToNative and ToSpeckle).
• For immediate compatibility the current Converter class will be persisted, serialising and deserialising as it currently does, however it will be expressly injected by consumers of the core converter class.
• Converters that will deal with their respective native types can be implemented and then consumed by the api, without the requirement of a separate explicitly named assembly and extension methods.

Step 0:

• I've read the contribution guidelines!

In progress. Every speckle object will have a .Scale( double factor ) method that will make unit conversions easier at a pre-geometry level.

This will not work for breps though.

Resulting in no returns as the server fails to parse them properly.

Following error is thrown if user tries to register with same email:

Might apply to gh and server too.

Wouldn't it make sense to use SpecklePoints instead of double[] Points { get; set; } for Curves etc.?
Name "Points" might be misleading as it's in reality Point coordinates and not SpecklePoints.