CURA 10561 makerbot plugin #4

jellespijker · 2023-10-11T14:09:03Z

Preparing the repo for our regular workflows and code style/standards.

The current plan is to create python binding using pybind11 and make a front-end plugin which will import this module in Cura as an bundled front-end plugin.

Significantly revised the project structure and made updates to the conanfile.py and CMakeLists.txt. The project was adjusted to follow a more standard layout, with all public headers moved to a central 'include' directory. The build system was updated to use more modern CMake practices and use the Conan package manager more effectively, removing the previous manual download of conan.cmake. Additionally, several source files were moved or renamed to support the changes in organization and convention. Extended the .gitignore file to ignore more unnecessary files created by the build system and IDE, and updated various #include directives in the source code to reflect the new file locations. The test setup in CMakeLists.txt was also simplified and improved for better integration with Google Test. These changes should provide a cleaner and more maintainable project structure, simplify the build process, and reduce potential for errors. It should also reduce the learning curve for new developers looking to contribute to the project. Contributes to CURA-10561

This update adds a new GitHub workflow for managing Conan packages. The workflow exports the recipe, sources, and binaries for Mac, Windows, and Linux and uploads them to the server where they can be used downstream. It also includes a new requirements file for the Conan package. The unit test workflow has been updated to include steps for installing dependencies and publishing test results. This will ensure consistent building and testing environments. Contributes to CURA-10561

github-actions · 2023-10-11T14:12:54Z

Unit Test Results

0 tests 0 ✔️ 0s ⏱️
1 suites 0 💤
1 files 0 ❌

Results for commit d1e86d5.

♻️ This comment has been updated with latest results.

This commit adds several GitHub workflows for automatic linting and formatting of the codebase. First, a "lint-formatter" workflow is added, which uses clang-format to automatically format .h, .cpp, and .h* files upon push. Then, a "lint-poster" workflow is added to handle displaying linting results after the completion of a "lint-tidier" workflow. Furthermore, a "lint-tidier" workflow applies the clang-tidy tool for automatic linting on pull requests for .h, .cpp, and .h* files. A python requirements file is included for the linter workflows, and standard clang configuration files for formatting and linting are also added. This change ensures consistent code styling and alerts for potential issues, which helps maintain the quality and readability of the codebase. Contributes to CURA-10561

The main change is a refactor of the style used for conditional blocks, method formatting, variable declaration, and namespace formatting in several files. This change makes the style consistent and easier to read and maintain. Furthermore, an unnecessary comment related to command-line utility in the CMakeLists.txt has been removed. The changes don't affect the functionality of the code, but should improve readability and ease of development. Made sure all necessary includes are present Optimized initialization of structs `constexpr`, `noexcept`, list initializers etc. Contributes to CURA-10561

The 'class' keyword was eliminated from CommandType and Tag enumerations in 'command_types.h'. Fixes warning: elaborated-type-specifier for a scoped enum must not use the ‘class’ keyword Contributes to CURA-10561

The include directives changed in several files: dulcificum/miracle_jtp/mgjtp_mappings_json_key_to_str.h, src/miracle_jtp/mgjtp_json_to_command.cpp, src/miracle_jtp/mgjtp_command_to_json.cpp, and include/dulcificum/miracle_jtp/mgjtp_json_to_command.h. All references to "dulcificum/command_types.h" have now been replaced with "dulcificum/miracle_jtp/mgjtp_command_to_json.h". This change was necessary as mgjtp_command_to_json.h provides more relevant functionality. Contributes to CURA-10561

Should fix the `error: use of enum ‘CommandType’ without previous declaration` Contributes to CURA-10561

Tag isn't constexpr Contributes to CURA-10561

This reverts commit e491845.

we use the conf file Contributes to CURA-10561

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/2)

include/dulcificum/command_types.h

include/dulcificum/miracle_jtp/mgjtp_mappings_json_key_to_str.h

src/miracle_jtp/mgjtp_command_to_json.cpp

test/miracle_jsontoolpath_dialect_test.cpp

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (2/2)

test/miracle_jsontoolpath_dialect_test.cpp

test/test_data_dir.h

scottsha

Could you edit the README to reflect your new build workflow? I'm a little uncertain of if I'm building correctly or having legitimate compiler complaints

apps/CMakeLists.txt

conanfile.py

test/test_data_dir.h

scottsha · 2023-10-12T00:49:22Z

include/dulcificum/command_types.h

+{
+    Command() = delete;
+
+    constexpr explicit Command(CommandType type) noexcept


I'm not familiar with the constexpr use on constructors. It will make class instances const right? What's the constexpr intent? The Command class instances are non constant. I'm having trouble compiling this

no it won't make them const, it will allow the instances to be constructed during compile time, if possible. If it's not possible it will be ignored.

My advice constexpr everything.

https://youtu.be/PJwd4JLYJJY?si=IFwB7xJkLEBpUeeP

jellespijker · 2023-10-12T01:20:22Z

you might want to do a new conan config install https://github.com/ultimaker/conan-config.git if you're having some compiler issues it is probably that the conf hasn't defined the compiler.cppstd=20 for this project. Installing the config should have updated profiles.

For building this should be enough

rm -rf build
conan install . --build=missing --update
cmake --preset release 
cmake --build --preset release

The 'translator' application has been added to the project. Relying on several libraries and offering command line options, it serves as primary user interface. The libraries used include 'docopt' for command line parsing, 'spdlog' for logging and 'jinja2' for templating. Build files 'CMakeLists.txt' have been updated accordingly to include applications and tests based on the new options. Besides, '.gitignore' has been updated to ignore generated header file 'cmdline.h'. This update also modifies 'conanfile.py' to include new requirements and options, as well as to generate 'cmdline.h' from a template. This streamlines the application build process, making it more flexible and maintainable. Contribute to CURA-10561

Python binding for Dulcificum bot library has been initialized. This includes necessary includes and setup in `pyDulcificum.cpp` file, modification in `CMakeLists.txt` for option to build with Python bindings, and updating the Conan file to include Python bindings option and requirements necessary for these bindings. This will allow Python scripts to directly use Dulcificum's bot commands and functionalities. Contributes to CURA-10561

according to code-style Contributes to CURA-10561

Contributes to CURA-10561

This commit adds the range-v3 library as a new dependency in the Conant and CMake files. This library is used to encapsulate iterators in ranged-based for loops for better readability and efficiency. It is specifically implemented in `mgjtp_json_to_command.cpp` and `mgjtp_command_to_json.cpp` files, where the traditional loops are replaced with ranged-based ones using `range/v3/view/zip.hpp` and `range/v3/view/remove_if.hpp`. This change simplifies the zipList functions in these files and improves code readability. Coontributes to CURA-10561

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

src/miracle_jtp/mgjtp_command_to_json.cpp

src/miracle_jtp/mgjtp_json_to_command.cpp

This is better supported by pybind11 out of the box, and it will also allow us to easily extend it for printers with more then 2 extruders. Contributes to CURA-10561

This commit introduces a new PlantUML file (griffin.puml) to illustrate the hierarchical structure of the UltiMaker g-code 'Griffin' flavor. It includes various classes like Griffin, Header, Body, Layer, Mesh, Feature, etc. along with their relationships and properties. This visual representation will aid in understanding and working with the Griffin gcode format. Contribute to CURA-10561

rburema

Synsepalum dulcificum is a plant in the Sapotaceae family, native to tropical Africa. It is known for its berry that, when eaten, causes sour foods (such as lemons and limes) subsequently consumed to taste sweet.

Some shade thrown? ;-)

rburema · 2023-10-13T15:39:40Z

doc/griffin.puml

@@ -0,0 +1,261 @@
+PlantUML example:


If we have the .puml file, so we then also need the (derived) .gif in the repo?

This is still work in progress, will be part of the final changes.
The derived png was already part of this PR: https://github.com/Ultimaker/synsepalum-dulcificum/pull/4/files#diff-5626f32ea1ca3ee3ccb4cae2bcac4ba021742b1dff9d25dcaa8fa20f867172d2 or are you adamant about the format?

rburema · 2023-10-13T15:45:20Z

include/dulcificum/command_types.h

+
+enum class CommandType
+{
+    kInvalid,


So in this enum-class (CommandType), the k... (for 'constant') prefix is used, but in the next one (Tag) that's omitted.

Since we use enum classes, the members really are part of the class, so we don't need to specify that they're a constant anymore like in the olden days, I think. But my point here is consistency, not which should be chosen.

(Unless there's another good reason for it, like these are legacy and used elsewhere or something ... but then a good idea to document that.)

not sure why these are named as such, I assume it part of the Makerbot codestyle preference.

These constructs will be part of a bigger overhaul later on, to allign with the structures needed for the GCode interpreter

Oversight. I was using magic enum at one point. Preference for following the google C++ style guide

rburema · 2023-10-13T15:55:02Z

include/dulcificum/command_types.h

+    Roof,
+    Support,
+    Sparse,
+    TravelMove


Just a stray thought, please ignore if inconvienient: Maybe drop ..Move from the name, so other things might be tagged travel, and it looks less like a command?

This will be renamed to Translate properly. If I end up using this.

naming scheme will probably reflect the same Word scheme as we use in the GCodeAnalyzer.

rburema · 2023-10-13T16:04:40Z

include/dulcificum/miracle_jtp/mgjtp_mappings_json_key_to_str.h

+namespace miracle_jtp::k_key_str
+{
+
+constexpr static std::string_view a{ "a" };


The use of static is redundant here; use inline constexpr instead if you want to do external linking.

rburema · 2023-10-13T16:05:36Z

pyDulcificum/pyDulcificum.cpp

+
+    py::class_<dulcificum::botcmd::Move>(module, "Move")
+        .def(py::init<>())
+        .def_readwrite("point", &dulcificum::botcmd::Move::point, "The point of the move command")


The point of the move command is to move from one point to the other :-p

try put will not be part of the final pr.

rburema · 2023-10-13T16:06:35Z

pyDulcificum/pyDulcificum.cpp

+        .def(py::init<>())
+        .def_readwrite("point", &dulcificum::botcmd::Move::point, "The point of the move command")
+        .def_readwrite("feedrate", &dulcificum::botcmd::Move::feedrate, "The feedrate of the move command")
+        .def_readwrite("is_point_relative", &dulcificum::botcmd::Move::is_point_relative, "Components of the point relative")


Missing part of the sentence here?

this is just a tryout for the bindings. This class will not end up in the final Python module.

rburema · 2023-10-13T16:24:50Z

src/miracle_jtp/mgjtp_command_to_json.cpp

+nlohmann::json getCommandParameters(const botcmd::Command& cmd)
+{
+    nlohmann::json jparams({});
+    if (cmd.type == botcmd::CommandType::kMove)


I think it would be better to have an overridable 'pack to parameters' function in the base-class. Why use inheritance if you're not going to use the benefits? ;-)

That way you get rid of the boiler-plate/casts.

(Also, I'd like you to change it, but if not, you could also use a switch or at least else if instead of plain ifs, since the cmd doesn't change.)

I'm currently working on a more generic Interpreter/parser based on Boost Spirit. So this probably change

The intent was to have a generic in-memory command zoo with translation functions to-from dialects. If dialect X doesn't have a direct version of a command Y, then you may need to replace Y with several commands more native to X. So I did not think overloads to every dialect was appropriate.

In general I don't think the generic command zoo should make reference to particular dialects directly, but rather that the dialects should specialize the zoo.

The intent is for an ordered list of commands, the inheritance here is to accommodate for the different data needs of the commands. We kept a concrete base because many commands do not require additional data past their command type, so we avoided a virtual base to avoid all the boiler plate class types that require no additional storage. But instead I suppose you could add a single virtual parent class to the tagged commands and have true polymorphism.

I think I misunderstood the json as a more generic output (for debug or saving), not its own dialect. It makes a bit more sense now.

Just to clarify: My point was not that I wanted you to use inheritance, or any solution specifically -- I should have been more clear; the thing that 'set me off' me is that explicitly checking for the type and acting on that can be a bit of a code-smell.

Yes def a smell, any thoughts on alternative patterns? I think I prefer an enum type over a proliferation of classes that carry no additional data

This pattern will probably be replaced by a visitor pattern (visiting the nodes in an AST), Since state is important when switching between languages which have different kind of verbosities The actual translation between languages is taken care of by a policy-based design idom. Working branch is: https://github.com/Ultimaker/synsepalum-dulcificum/tree/CURA-10561_regex_gcode_parser

Agree that a visitor pattern would probably be best in this case, yes.

sounds good

src/command_types.cpp

jellespijker · 2023-10-13T16:53:15Z

@rburema thanks for looking at this bit, the request for the review was for the logistic part. The workflows and build scripts.

You're currently review code not written by me, which will be overhauled.

rburema

I accidentally another review.

Anyway I can't compile it on windows, as either I or some package this needs uses the old MSBuild toolchain (I think? From reading the bugreports on it), as I get:


Installing (downloading, building) binaries...
ERROR: There are invalid packages (packages that cannot exist for this configuration):
cpython/3.10.4: Invalid ID: Static msvc build disabled (>=3.10) due to "AttributeError: module 'sys' has no attribute 'winver'"
PS C:\dev\synsepalum-dulcificum>

When trying to install it (I even upgraded to conan 1.61 and installed a new config). Too braindead at the moment to investigate/do a full wipe though.

.github/workflows/conan-package.yml

.github/workflows/lint-tidier.yml

templates/cmdline.h.jinja

This commit removes template file generation for cmdline.h.jinja and adds direct definitions in CMakeLists.txt and related files. The changes are necessary to improve the handling of name and version management across the application. This change leads to easier maintenance by having one single point of truth for the application version and building it directly into the binaries during the compile time. App names and versions are now defined using compile definitions in CMakeLists.txt. The .gitignore file is also updated to stop ignoring cmdline.h as it is no longer auto-generated. Contributes to CURA-10561

The Python bindings setup was moved from the main CMakeLists.txt file into a new separate CMakeLists.txt within the pyDulcificum directory. This was done to modularise the CMake files and make the sections easier to manage and understand. This approach also provides better isolation of the configuration of different components. Contributes to CURA-10561

Contributes to CURA-10561

github-actions

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

include/dulcificum.h

jellespijker · 2023-10-15T11:31:31Z

@rburema I created a second branch in which I start working on a GCode interpreter, based on this branch (see: CURA-10561_makerbot_plugin...CURA-10561_gcode_interpreter). I applied most of the suggestions from your second review and I will create a C&C ticket, about your remarks about the Python version etc. Because I really like the idea of having a single source of truth for these kind of variables/options across all our repo's. I would suggestion that we leave it be in this PR. It follows the same conventions that we already use. If we decide on a new convention in the C&C we can change this repo in line with al our repo's.

If you're okay with this than I think we should merge this and I can make a new PR for the GCode interpreter and after that for the translation vector between GCode <-> Makerbot.

will be addressed at a later time

rburema · 2023-10-15T14:34:43Z

@jellespijker Yeah sounds fine! I knew I had seen this code before, but this gave me a chance to address some of the wishlist type things I saw. We can fix the windows thing later, also since it might be a problem on my end. I'm not sure how much commits you still want to make on this branch, so if you're ready, feel free to merge.

jellespijker · 2023-10-15T19:19:44Z

same error you're experiencing seems to occur on the Windows runner. Which only ran when merged to main. I do think we can rule out that it a personal local issue for @rburema then.

I will investigate further after I finished the gcode interpreter PR.

jellespijker added 2 commits October 11, 2023 15:55

jellespijker and others added 10 commits October 11, 2023 17:24

Applied clang-format.

a761b3d

Remove 'class' keyword from enums

ab138dc

The 'class' keyword was eliminated from CommandType and Tag enumerations in 'command_types.h'. Fixes warning: elaborated-type-specifier for a scoped enum must not use the ‘class’ keyword Contributes to CURA-10561

Applied clang-format.

d54acd0

revert uint8_t enum

e7167e7

Should fix the `error: use of enum ‘CommandType’ without previous declaration` Contributes to CURA-10561

Removed constexpr

e491845

Tag isn't constexpr Contributes to CURA-10561

Revert "Removed constexpr"

4f6d7e7

This reverts commit e491845.

no test options available

2b0a4de

we use the conf file Contributes to CURA-10561

github-actions bot requested changes Oct 11, 2023

View reviewed changes

jellespijker requested a review from scottsha October 11, 2023 17:23

scottsha suggested changes Oct 12, 2023

View reviewed changes

jellespijker added 6 commits October 12, 2023 10:56

improved readability

e0bb17c

according to code-style Contributes to CURA-10561

Don't use namespace using-directives

799b852

Contributes to CURA-10561

Fixed strip target

924a03c

Contributes to CURA-10561

github-actions bot requested changes Oct 12, 2023

View reviewed changes

jellespijker added 2 commits October 12, 2023 14:42

Use vector instead of array

5bbc205

This is better supported by pybind11 out of the box, and it will also allow us to easily extend it for printers with more then 2 extruders. Contributes to CURA-10561

rburema requested changes Oct 13, 2023

View reviewed changes

rburema reviewed Oct 13, 2023

View reviewed changes

jellespijker added 5 commits October 15, 2023 12:58

Applied code-review suggestions

22b397a

Contributes to CURA-10561

Remove Runner cache hack

6baad99

Contributes to CURA-10561

Remove Runner cache hack

d1e86d5

Contributes to CURA-10561

github-actions bot previously requested changes Oct 15, 2023

View reviewed changes

include/dulcificum.h Show resolved Hide resolved

jellespijker requested review from rburema and scottsha October 15, 2023 12:58

rburema approved these changes Oct 15, 2023

View reviewed changes

jellespijker merged commit a806ed4 into main Oct 15, 2023
5 checks passed

rburema deleted the CURA-10561_makerbot_plugin branch October 15, 2023 21:44

CURA 10561 makerbot plugin #4

CURA 10561 makerbot plugin #4

Conversation

jellespijker commented Oct 11, 2023 • edited Loading

github-actions bot commented Oct 11, 2023 • edited Loading

Unit Test Results

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

scottsha left a comment

Choose a reason for hiding this comment

scottsha Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jellespijker commented Oct 12, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

rburema left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jellespijker Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rburema Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottsha Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jellespijker commented Oct 13, 2023

rburema left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

jellespijker commented Oct 15, 2023 • edited Loading

rburema commented Oct 15, 2023

jellespijker commented Oct 15, 2023

jellespijker commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023 •

edited

Loading

scottsha Oct 12, 2023 •

edited

Loading

jellespijker Oct 13, 2023 •

edited

Loading

rburema Oct 13, 2023 •

edited

Loading

scottsha Oct 17, 2023 •

edited

Loading

jellespijker commented Oct 15, 2023 •

edited

Loading