Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toolsocket updates #1031

Merged
merged 10 commits into from
Apr 11, 2024
Merged

Toolsocket updates #1031

merged 10 commits into from
Apr 11, 2024

Conversation

dangond-ptc
Copy link
Contributor

  • Adapted codebase to use ToolSocket v2.0.0's API.

@dangond-ptc
Copy link
Contributor Author

I'm still working on tracking down the cause of the failing tests.
This PR should not be merged until the ToolSocket repo officially updates to 2.0.0.

@dangond-ptc
Copy link
Contributor Author

dangond-ptc commented Apr 9, 2024

Ok, after much investigation: the current code passes all tests on a clean install with the right toolsocket dependency. The issues I was having are:

  1. chromium-browser must be installed for puppeteer to work, can be installed on MacOS with brew install chromium and Ubuntu with sudo apt-get install chromium-browser
  2. The test tests/data-streams.test.js fails on my laptop for some unknown reason. This is true for the current main branch as well. I tested on an Ubuntu machine instead, and all tests pass there. Nothing else seems to be different between the two machines other than my laptop has a Documents/spatialToolbox directory, which, from what I can tell, should not affect this test.
    In short, tests should pass once toolsocket is updated and the tests are re-run.

@dangond-ptc dangond-ptc marked this pull request as ready for review April 9, 2024 14:21
@@ -75,7 +75,7 @@ const {identityFolderName} = require('../constants.js');
var hardwareIdentity = path.join(objectsPath, identityFolderName);

let socketReferences = {
realityEditorUpdateSocketArray: null
realityEditorUpdateSocketSubscriptions: null
};

let ioReference = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can drop this variable and its use in utilities.setup()

for (var thisEditor in realityEditorSocketArray) {
realityEditorSocketArray[thisEditor].forEach((thisObj) => {
if (typeof sourceSocketID !== 'undefined' && thisEditor === sourceSocketID && msgContent.object === thisObj.object && msgContent.frame === thisObj.frame) {
function sendMessageToEditors(msgContent, sourceSocket) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great improvement in readability!

@hobinjk-ptc
Copy link
Contributor

(fix lint errors before merging)

Copy link
Contributor

@benptc benptc left a comment

Choose a reason for hiding this comment

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

LGTM

server.js Outdated
Comment on lines 3480 to 3482
const subscriptions = [];
subscriptions.socket = socket;
realityEditorSocketSubscriptions.push(subscriptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Little bit confused about adding a socket property to an array, rather than using an object, is there a reason to structure it like this? (I'm not always up-to-date with clean coding conventions so I figure you have a valid reason, just curious)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at it now, I cannot come up with a good answer for this. I'll push a commit with a more sensible arrangement!

Comment on lines +79 to +83
```bash
git clone --recurse-submodules https://github.com/ptcrealitylab/vuforia-spatial-edge-server.git
cd vuforia-spatial-edge-server
./scripts/ci.sh
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating the README, too!

@dangond-ptc
Copy link
Contributor Author

@benptc Added a new commit regarding the socket property on the arrays. Lmk if that's better and looks good!

@dangond-ptc dangond-ptc merged commit d096953 into main Apr 11, 2024
3 checks passed
@dangond-ptc dangond-ptc deleted the toolsocketUpdates branch April 11, 2024 18:07
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