Open Millyarde opened 1 year ago
Data
classes, so there doesn't appear to be any benefit in having them inherit from a common ancestor.this
is a holdover from my day job as a JavaScript monkey. Because of the IDE color palette I use, prepending things with this
makes them pop on a way that's easy for me to read. Additionally, Unity does not support the null-coalesce operator well on things that can be serialized in the Inspector, for reasons that have been lost to the sands of time. I also generally think that the null-coalesce operator is a lot easier to miss than an if-block when skimming over code.dynamic
does. ๐
Hope that helps address things!
[Description]
tag of the API, so that we have a 'strongly typed' reference to the message types, as opposed to the current scattered 'magic strings' of the message type, in case they change one or something in the future. Does add some degree of overhead since you have to use Reflection to grab the string off the enum instead of just... using a string, but it's pretty minor overhead for the safety and usability it provides imo.[Serializable]
tag is only for Binary Serialization and has nothing to do with JSON Serialization. Totally understand wanting to avoid version conflicts as well, Streamer.bot (which I'm trying to use your library to make a clean extension for it), has it's own version of WebsocketServer and Newtonsoft, dealing with versioning with C# is headaches for days ๐ญany
in Typescript, except with performance drawbacks greater than reflection even ๐คฎ, definitely a no-no here, but it has it's uses in some places.So it seems like "No"'s on everything?
Serializable
attribute.We could just do const strings instead of an enum as well for the message types, that's a little less overhead than the enum description tag approach. Would lose a little bit of code flexibility, but it is a little more performant.
Yeah, the other thing is that outgoing messages all need to be SomethingRequest
and the incoming messages are all SomethingResponse
. Unless we wanted two unlinked enum values each, we could probably get away with making the enum value just Something
and having the Send
method append the word Request
to it. Similarly, the receiving switch could attempt to trim Response
from the string value to find the matching enum.
I wish C# allowed for complex enum objects like Java does. That's the one word of praise I'll give the language.
EDIT: I'm just now comprehending that we can just make two custom attributes:
enum MessageTypes {
[Request("SomethingRequest")]
[Response("SomethingResponse")]
SOMETHING,
}
This is functionally done. I'm kicking the enum-ification down the road a bit, but you can use the code from the branch: https://github.com/FomTarro/VTS-Sharp/tree/unity-decouple safely now. I do not anticipate further changes. I am presently writing the migration steps in the README, which you presumably do not need for your purposes.
Sorry for the month of delay.
Sounds good! I'll take a look at the changes and hopefully work on some stuff with this next week. Saw the to/from JSON abstraction, Newtonsoft addition, and removing the meta files though, looks great and will make things easier for my little fork!
No worries on taking a while, figured this would be a longer work in progress and some stuff I could help add/code review if it was stuff you wanted ๐
I got busy myself so I hadn't really had a chance to work on stuff anyway. We can close this out if you don't like seeing the "1 issue" and I can just send in PR's for stuff like the enums if/when I get to it. Whatever works best for you, thanks for the hard work ๐
The 2.0.0 release is officially out. 2.1.0 will begin a serious investigation of enumerating message types and deprecating the use of strings as much as possible.
[ ] Enums for MessageType, Add Enum Extension Methods
We can use Extension Methods to get a [Description] tag off of an Enum. This adds a little overhead, since we have to get the string at runtime as opposed to just reading the string.
Example:
Pros: Less typos, easier to develop/maintain.
Cons: Less performant. Not sure if that is an issue, since it is all (usually) running on a local network.
VTSData
as[Serializable]
. This is marking a class being Binary Serializable in C# and is generally frowned upon for various reasons, especially since we just need to serialize it for JSON data.Pros: Not using [Serializable].
Cons: Adding a library (Newtonsoft) to the project.
[ ] Rework Data, remove Data from subclasses, merge onto VTSMessageData I was thinking we could just use Inheritance and Generics here to not create a new
Data
on every class. Here is an example of howVTSMessageData
andErrorData
could lookPros: Code is a bit easier to work with and expand upon.
Cons: Maybe a little less understandable at a glance? Currently need to use Static Usings, IE:
using static VTS.VTSArtMeshListRequestData;
to access the 'Data' types, which will clutter usings in some files. I'll try to think of a better way to handle this though.this.
, variable name casing, null coalescing, null propagation, and other VERY MINOR things. This isn't so much a problem, but thought I'd bring it up and see what your thoughts on it were. I could make these changes in a pull request, but wanted to see if this was overstepping my bounds a little.Example: This:
Becomes:
_ws?.Update(timeDelta);
Pros: My Brain will process this better?
Cons: Your/Other People's Brains may not?
ProcessResponses
, allowing for not having to manually update this whenever we add a new type. This example also uses(dynamic)
, which I would like to refactor out, since this also adds some extra overhead along with the use of Reflection. I just haven't thought of a better way of doing this yet.Example:
Pros: Easier to maintain.
Cons: Uses more computation.
Please let me know if any of these possible enhancements interest you! I'll make some pull requests with these features separately. I already have some of this work done in one, gross, big, commit I was working on as a test over at https://github.com/Millyarde/VTS-Sharp/commit/e3851fba2f6426e7744c592c5433ade62a725dfd. I'll probably wait until version 2 is in main to make these changes, if that isn't too far off!