Skip to content

Conversation

@YanehCheck
Copy link
Contributor

Introduction

The library mostly replaces the WebSocketManager class, which has been renamed to CommunicationManager and now holds the library client. I decided to retain its responsibility of invoking other Manager's methods on events, similarly to the previous architecture.

The library also eliminates the need for the NativeWebSockets package, which has as such been removed from the git submodules.

Inclusion of the Library

The library is included by referencing an assembly file (similar to the now-removed OpenApi model assembly). I chose this approach to avoid adding an extra third-party dependency for some implementation of NuGet package manager for Unity. Newer versions of the assembly file will later be automatically generated through the CD/CI pipeline on the library repository, making updates as simple as replacing the assembly file.

Conversion of Existing Code

Most changes listed in the diff are mainly renames. Code that previously expected methods with a callback parameter has been updated to use asynchronous methods or Task continuations. A dispatcher object was added, as all events from the library are generated on a non-main thread.

Testing

Each action that involves communication with the ARCOR2 server has been manually tested, and everything seems to behave as expected.

Let me know if I should change or further update anything!

@ZdenekM ZdenekM requested a review from Kapim March 10, 2025 09:42
@ZdenekM
Copy link
Member

ZdenekM commented Mar 10, 2025

@YanehCheck Looks clean to me. I requested the review from @Kapim, the author of the original code. And can we see the library code somewhere? I will create a new repository for it - something like robofit/arcor2_client_sdk?

@ZdenekM
Copy link
Member

ZdenekM commented Mar 10, 2025

@Kapim At the moment, the library is here: https://github.com/YanehCheck/Arcor2.ClientSdk/tree/master. To me, it looks well-documented and structured.

Copy link
Collaborator

@Kapim Kapim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok to me. Good job!

@ZdenekM ZdenekM merged commit 52d16cc into robofit:master Apr 28, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants