clicketyclackety / Crash

A multi-user implementation of Rhino3d
MIT License
37 stars 5 forks source link

Updated Speck implementation #19

Closed clicketyclackety closed 1 year ago

clicketyclackety commented 1 year ago

Description

Currently there are 2 speck implementation, 1 local and 1 server-side. This was okay for the hackathon, but the current implementation has quite the performance penalties.

I've implemented an ISpeck interface to allow for more flexibility in server/client side handling of them. In the local implementation, it de-serializes and caches the geometry to prevent constant, slow de-serialization. In the server implementation it has all of the information it had before, but with a few different constructors.

Type of change

Please delete options that are not relevant.

clicketyclackety commented 1 year ago

@cwensley, @rfeathers068 I can't quite work out why this doesn't work. And I don't think I'm debugging it correctly. Everything builds, but the specks aren't passed around correctly. Would love a hand with this one, either fixing, or suggesting a better implementation. 😋

rfeathers068 commented 1 year ago

@cwensley, @rfeathers068 I can't quite work out why this doesn't work. And I don't think I'm debugging it correctly. Everything builds, but the specks aren't passed around correctly. Would love a hand with this one, either fixing, or suggesting a better implementation. 😋

One thing I noticed, which Curtis may have to help with is the Database Migration didn't update for me so that might be part of the issue as well.

image

clicketyclackety commented 1 year ago

One thing I noticed, which Curtis may have to help with is the Database Migration didn't update for me so that might be part of the issue as well.

image

I had a feeling this might play a part

cwensley commented 1 year ago

Hey @clicketyclackety, a few things that I can see about this PR:

  1. Why is ServerSpeck in SpeckLib but LocalSpeck is in Crash? I would expect ServerSpeck to be in Crash.Server unless there's a reason for that? Ideally we should just use the same class on both sides and if we need other information on either side we can wrap it in an outer class.
  2. I'm not sure you can use an Interface to pass things around with SignalR.. what class would it create on the server if it knows nothing about LocalSpeck? It does not have any knowledge to create a ServerSpeck or LocalSpeck.
  3. Any database model changes need to be reflected with a dotnet ef add migration <name_of_migration> in the Client.Server\ folder. I should add a VS Code task for that and some instructions to the docs.
  4. The ServerSpeck class requires a public default constructor for it to be used by the Entity Framework database, otherwise there's no way for the database calls to create instances of the class.

That's all I can see for now, I hope this helps!

clicketyclackety commented 1 year ago

Hey @cwensley,

Thanks for the notes. I was able to get things running again with a few changes;

  1. ServerSpeck gets used by Crash to convert into LocalSpeck so that geometry is cached, which should fix a lot of performance issues. LocalSpeck uses GeometryBase and I don't want the server to have to use Rhino dlls. I don't like the separation, it feels, less than ideal. If there's a more correct way to do things I'd appreciate the input 😊
  2. That is really handy to know. Makes total sense. I've moved to inheritance
  3. Handy command. Would love a script for it. For now I've just been cleaning the directories and I updated the initial migration
  4. I've ensured default constructors and getters/setters. Everything is currently public.
clicketyclackety commented 1 year ago

I think likely you guys may spot some more things. But it does now work nicely! Much nicer to be able to connect to the server and retrieve your work in progress stuff 😊

clicketyclackety commented 1 year ago

@cwensley if you could take a look at the new implementation, that'd be fab 😊