search

specklesystems / speckle-unreal

Unreal Engine 4/5 plugin for importing objects from Speckle v2.
https://speckle.systems/tag/unreal/
Apache License 2.0
54 stars 14 forks source link

Speckle-UE5 Fetch List of Streams and List of Branches #58

Closed jimver04 closed 2 years ago

jimver04 commented 2 years ago

Hi Jedd,

Can you please merge my developments ? The List of Branches and List of Streams can be fetched now as follows:

image

System: Rider 2022.1.1 Windows 10 UE5.0.1

More developments to follow for Collaborators, Activities, and other

jimver04 commented 2 years ago

Sorry I have not pushed the latest. Just a minute

jimver04 commented 2 years ago

Now I have the latest with both Streams and Branches pushed to github. You can make the merge. I think it has no conflicts as I have merged your latest in my git.

jimver04 commented 2 years ago

Hi Jedd, I have done some new commits today for this branch I guess they are automatically added to the PR.

JR-Morgan commented 2 years ago

Hi @jimver04,

I've checked out your changes. I'm really happy!

I do have a couple thoughts. I'm thinking it might make sense to separate the stream/branch/commit/etc fetching from the transports, and keep transports just for fetching objects.

I'm thinking that could tidy up the inputs for the nodes a fair bit. Something like this maybe?:

image

I've started to make this change on my local branch, as well as some other minor clean-up/cross platform bug fixes. If you're happy, I can go ahead and merge this feature in! 🥳 (once I've given my changes a bit more testing)

jimver04 commented 2 years ago

Hi Jedd, I was thinking that having speckle stream/branches objects in a memory transport would make it easy to reuse them rather having to store them in custom variables. Server transports are also a compact way to have all things in one object. Now with this above we have to put more variables as input which makes visual programming too big.

JR-Morgan commented 2 years ago

Hi @jimver04

I'm not sure I fully understand why you are needing to cache streams/branches/commits locally.

But I have some concerns about using transports for this:

  1. Transports should be able to be used abstractly, i.e. the function using a transport, shouldn't need to know what type of transport it is. Since branches/streams are mutable, a memory transport would have no way to know if the data it has is up-to-date and every caller is now responsible for ensuring the data is up to date, therefore needs to know the type of transports it's using.

  2. I speed is your concern, the GraphQL requests for branches/streams are fast, because it's a very small amount of data, compared with fetching objects. I'm measuring sub second speeds. If you need to cache, then you can always create your own variable in the scripts where it's needed.

  3. One thing I would like to mention is, I would quite like to implement an SQLite transport at some point - for interfacing with the local Objects db that most of our other connectors use for locally caching objects. This means it's important that I keep transports aligned with our .NET, and PY SDKs.

Expanding transports to store streams/branches/etc would require a change in all of our SDKs and Connectors. This is something I'm happy to discuss more, and a discussion we can perhaps have with the wider team. But for the minute, I want to keep transports aligned with .NET.

That being said, there are other ways we can make the input situation better.

If we were to combine Server url + Auth token ( + maybe some extra account/user info) in a "client" object, similar to what we do on the .NET side. This would reduce the number of inputs required.

jimver04 commented 2 years ago

  1. ok As regards Globals, I will remove the commit id. Fetch only the latest commit in Globals is what is needed. What do you think ? I will hide the double quering in c++.

  2. ok

  3. ok

  4. Procedural meshes have their normals reversed. Perhaps the Procedural mesh component has triangles that are CCW generated instead of CW? image See this resource: https://www.orfeasel.com/creating-procedural-meshes/#:~:text=At%20this%20point%2C%20it%20is%20important%20to%20note%20that%20the%20way%20you%E2%80%99re%20adding%20your%20vertices%20in%20the%20triangles%20array%20matters

  5. I will expose also Layer names (Rhino only) into Receive commit in UE5. We have to identify 1-to-1 what corresponds to what in Rhino.