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

NP- 327 emscripten communication #2131

Merged
merged 36 commits into from
Aug 30, 2024
Merged

Conversation

jellespijker
Copy link
Member

@jellespijker jellespijker commented Aug 7, 2024

Description

Add an extra communication option when using Emscripten and compiling for wasm. This is based on the cmdline communication layer and uses a callback function to be executed on the JS side, providing slice information and possibly in the future preview information.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Locally UT, for Arcus, Commandline and Emscripten.
  • In the CuraEngineJS

Test Configuration:

  • Operating System: wasm

Checklist:

  • My code follows the style guidelines of this project as described in UltiMaker Meta
  • I have read the Contribution guide
  • I have commented my code, particularly in hard-to-understand areas
  • I have uploaded any files required to test this change

jellespijker and others added 10 commits August 1, 2024 10:12
Refactored conditional compilation to enable Emscripten-specific behavior in the communication module. Updated CMakeLists.txt to handle Emscripten build settings and refactored EmscriptenCommunication to properly manage progress handling and command-line arguments.

Contribute to NP-327
This commit adds support for handling slice info callbacks through Emscripten, alongside progress callbacks. Additionally, functions to create slice info messages using rapidjson were implemented to prepare JSON data for slice information.

Contribute to NP-327
Replaced raw pointers with `std::shared_ptr` for better memory management. Standardized variable names to follow consistent naming conventions, improving code readability and maintainability.

This was needed because the reference to Slice was otherwise out-of-scope when calling `Emscripten::sliceNext()`

Contribute to NP-327
Replaced unique_ptr with shared_ptr in various components to facilitate shared ownership of resources and prevent manual memory management. This change affects communication objects, `Slice` instances, and related test cases to ensure smooth integration.

Contribute to NP-327
@jellespijker jellespijker marked this pull request as ready for review August 7, 2024 08:24
Copy link
Contributor

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

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

Oooooooooh
Nice improvement, some minor remarks but apart from that I do like it very much 👍

include/Application.h Outdated Show resolved Hide resolved
include/Application.h Outdated Show resolved Hide resolved
include/communication/EmscriptenCommunication.h Outdated Show resolved Hide resolved
src/Application.cpp Show resolved Hide resolved
src/communication/ArcusCommunication.cpp Show resolved Hide resolved
src/communication/CommandLine.cpp Outdated Show resolved Hide resolved
src/communication/EmscriptenCommunication.cpp Outdated Show resolved Hide resolved
stress_benchmark/stress_benchmark.cpp Outdated Show resolved Hide resolved
Added detailed documentation comments for the EmscriptenCommunication class, including descriptions for member variables and methods. This improves code readability and ease of maintenance.

Contribute to NP-327
Contribute to NP-327
The return type of getTotalExtraLayer function on Raft is casted to the value_type to prevent underflow. This previously resulted in method xl to generate invalid gcode.
@0x5844
Copy link
Contributor

0x5844 commented Aug 20, 2024

6697713 is commited to handle NP-343 "Slicing for a method XL produces unexpected results" where the generated gcode was empty. The code change is under #2134

@wawanbreton
Copy link
Contributor

There has been many commits on this PR recently, is it ready for review again ? Unit tests are failing though.

Contribute to NP-327
…roduces-unexpected-results' into NP-327_emscripten_communication
@jellespijker
Copy link
Member Author

@wawanbreton This now also integrate #2134 since we're building our Working package of this branch

Copy link
Contributor

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

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

Just one nitpicky remark about naming, which got me confused. Otherwise looks good.

include/communication/EmscriptenCommunication.h Outdated Show resolved Hide resolved
Renamed `gcode_prefix_handler_` to `gcode_header_handler_` for clarity. Updated corresponding usage in `EmscriptenCommunication.h`, `CommandLine.cpp`, and `EmscriptenCommunication.cpp`.

Contribute to NP-327
saumyaj3 and others added 4 commits August 22, 2024 11:27
Ensure the prefix argument is correctly formatted as a string in JavaScript function calls. This change prevents potential issues with GCode prefix handling in the communication module as it starts with ';'

NP-327
…r/CuraEngine into NP-327_emscripten_communication
@0x5844
Copy link
Contributor

0x5844 commented Aug 22, 2024

@wawanbreton @jellespijker I removed the type cast; I gave it additional tests and it seems that the WASM_BIGINT flag is sufficient to handle slicing issues with method xl.

saumyaj3 and others added 5 commits August 22, 2024 15:32
Implemented a `convertTobase64` function to encode strings to base64, ensuring they can be safely transmitted in JavaScript. Updated `EmscriptenCommunication::sendGCodePrefix` to use this new encoding method and modified relevant tests accordingly to check this functionality.

NP-327
Co-authored-by: Erwan MATHIEU <erwan.mathieu@ultimaker.com>
assuming that cloud slicer does not create multiple mesh groups, but one.

NP-351
@wawanbreton
Copy link
Contributor

What is the status of this PR ? There has been a few significant changes since the last approval (which I would ask you to avoid as much as possible), is it now stable enough for a final review ?

@jellespijker
Copy link
Member Author

Yes it can be reviewed as final

@casperlamboo
Copy link
Contributor

casperlamboo commented Aug 27, 2024

@wawanbreton @jellespijker I removed the type cast; I gave it additional tests and it seems that the WASM_BIGINT flag is sufficient to handle slicing issues with method xl.

With these proposed changes I am still able to reproduce the method xl slicing issues. I think the cast is still needed for wasm. (FYI @0x5844)

@0x5844
Copy link
Contributor

0x5844 commented Aug 27, 2024

@wawanbreton @jellespijker I removed the type cast; I gave it additional tests and it seems that the WASM_BIGINT flag is sufficient to handle slicing issues with method xl.

With these proposed changes I am still able to reproduce the method xl slicing issues. I think the cast is still needed for wasm. (FYI @0x5844)

That's strange, if we want to keep that cast we have to adjust it in all it's occurrences (I don't think this is the way to go about it). Currently there are multiple changes to how we compile wasm (flags passed to the compiler); I would much rather wait for the merge and have a fresh test again. Removing the typecast while enabling the compiler flag seemed to tackle to issue (?) which seem to be no longer the case (this was tested last week).

@HellAholic HellAholic merged commit 0270dce into main Aug 30, 2024
27 checks passed
@HellAholic HellAholic deleted the NP-327_emscripten_communication branch August 30, 2024 07:03
@saumyaj3 saumyaj3 restored the NP-327_emscripten_communication branch October 7, 2024 11:40
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.

6 participants