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

UFPWriter: fix serialization of SettingFunctions #13473

Merged
merged 1 commit into from
Oct 5, 2022
Merged

UFPWriter: fix serialization of SettingFunctions #13473

merged 1 commit into from
Oct 5, 2022

Conversation

Piezoid
Copy link
Contributor

@Piezoid Piezoid commented Oct 4, 2022

PR #13209 adds the modified settings to the UFP exports. The settings are serialized as JSON but doesn't handles the SettingFunctions. This was also reported by users in emtrax-ltd/Cura2MoonrakerPlugin#55.

Reproduction steps:: Modify a setting with a computed default, then click the image icon. Slice and save the result as a .ufp file. Cura will crash with:

Traceback (most recent call last):
  File "Cura/cura/CuraApplication.py", line 1127, in event
    return super().event(event)
  File "Uranium/UM/Qt/QtApplication.py", line 502, in event
    event._function_event.call()
  File "Uranium/UM/Event.py", line 218, in call
    self._function(*self._args, **self._kwargs)
  File "Cura/cura/Utils/Threading.py", line 34, in _handle_call
    ico.result = func(*args, **kwargs)
  File "Cura/plugins/UFPWriter/UFPWriter.py", line 87, in write
    json.dump(self._getSliceMetadata(), setting_textio, separators=(", ", ": "), indent=4)
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/__init__.py", line 179, in dump
    for chunk in iterable:
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/encoder.py", line 431, in _iterencode
    yield from _iterencode_dict(o, _current_indent_level)
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/encoder.py", line 405, in _iterencode_dict
    yield from chunks
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/encoder.py", line 405, in _iterencode_dict
    yield from chunks
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/encoder.py", line 405, in _iterencode_dict
    yield from chunks
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/encoder.py", line 438, in _iterencode
    o = _default(o)
  File "$HOME/.conan/data/cpython/3.10.4/_/_/package/9054a82b60899c3ed90865d5b990fd99ef2dc167/lib/python3.10/json/encoder.py", line 179, in default
    raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type SettingFunction is not JSON serializable

This patch mimics what GcodeWriter does, calling str() on the setting values.

@jellespijker
Copy link
Member

@Piezoid good catch can ypu merge this against the 5.2 branch though

@jellespijker
Copy link
Member

@Joeydelarago can you do the review and component test and create a jira ticket for this since you qorked on that feature

@Piezoid Piezoid changed the base branch from main to 5.2 October 4, 2022 21:28
@casperlamboo casperlamboo merged commit d9d2488 into Ultimaker:5.2 Oct 5, 2022
@nallath
Copy link
Member

nallath commented Oct 6, 2022

Hmm, this isn't quite correct! In this case the values need to be "flattened" so that you get key value pairs (without any setting functions in it).

if isinstance(property_value, SettingFunction):
    property_value = property_value(global_stack)

@Piezoid
Copy link
Contributor Author

Piezoid commented Oct 6, 2022

Hmm, this isn't quite correct! In this case the values need to be "flattened" so that you get key value pairs (without any setting functions in it).

Yes, I don't know anything about the receiving end for this metadata, that is why I asked if this solution was ok. I went for the simplest solution since I wasn't aware that SettingFunction could be evaluated that way.

@Piezoid
Copy link
Contributor Author

Piezoid commented Oct 6, 2022

@nallath BTW, I think that the image action should clear the setting override if the default setting value is that same function. Currently a setting reset with this button will be shown in the changes list.

@nallath
Copy link
Member

nallath commented Oct 6, 2022

Yeah, no worries. We will change it. We should have done this from the start, but we missed it...

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.

4 participants