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

Plugin gcode modify patches #1979

Merged
merged 10 commits into from
Sep 30, 2024

Conversation

ThomasRahm
Copy link
Contributor

Description

CuraEngine currently does not set per model settings to a the GcodePathModify slot.
Also if fan speed is changed for travel paths, the changed fan speed will not be applied.
This fixes both of these issues.
Also it adds the a "mesh_name" setting to the per-model settings, so that the mesh can later be identified using its name.
To my knowledge the mesh name is unique as if the same model is added multiple times a counter is appended by the frontend (e.g. Example.stl and Example.stl(1) and so forth).

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Nov 9, 2023
@casperlamboo
Copy link
Contributor

Resolved the merge conflicts from the github website ui, soooo if i have broken anything sowry

@casperlamboo
Copy link
Contributor

casperlamboo commented Jan 31, 2024

Hi @ThomasRahm ,

Thanks for you contribution! Heard about this oversight indeed from @rburema, good initiative to fix it!

I have a question though

CuraEngine currently does not set per model settings to a the GcodePathModify slot.
...
Also it adds the a "mesh_name" setting to the per-model settings, so that the mesh can later be identified using its name.

This last remark seems to me a bit like a workaround of the issue above. If we were to send over the the model_id with the modify call, then we would be able to retrieve the correct settings from the correct model setting stack?

@ThomasRahm
Copy link
Contributor Author

It definitely is a bit of a workaround.

Every path seems to be able to belong to a different model, so it would need to be sent as part of each path.
Also I do not know if the RepeatedPtrField returned by BroadcastServiceSettingsRequest::object_settings() is ordered (As in the settings of model 3 are at position 3).
A model id is in my opinion better to identify models than the name, I just do not know how I would be extending the API to include it.

Also want to note that this pull request merges (for plugins) mesh groups settings with the per object settings on engine side before they are being sent to the plugin. If this is not wanted one would even need to also transmit an identifier for the mesh group the model is part of and transmit the mesh group settings separately.

@jellespijker jellespijker added the PR: Slicing Process 🤯 Like fixing simplification, adding a new primetower, introducing tree support, correcting logging, label Feb 16, 2024
@jellespijker
Copy link
Member

Hi Thomas,

We're in the progress of updating our Way-Of-Working with regards to PR's so I'm adding some label(s). That way we can prioritize our work-load and give your PR the love it deserves.

Either I our one of my colleagues will pick this up in the near future, but I think @casperlamboo is already doing this

Copy link
Member

@nallath nallath left a comment

Choose a reason for hiding this comment

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

LGTM

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.

This looks a bit like a workaround for something we should fix otherwise, but I don't see an easy enough way for it...

@HellAholic HellAholic merged commit a4cb6be into Ultimaker:main Sep 30, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Community Contribution 👑 Community Contribution PR's PR: Slicing Process 🤯 Like fixing simplification, adding a new primetower, introducing tree support, correcting logging,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants