jeffeb3 / sandify

web based user interface to create patterns that could be useful for robots that draw in sand with ball bearings.
MIT License
187 stars 34 forks source link

Add option to export a png preview of the path #225

Closed geekuillaume closed 11 months ago

geekuillaume commented 2 years ago

This PR adds an option to export the canvas preview as a PNG file in addition to the path file.

Fixes #218

jeffeb3 commented 2 years ago

This is great. Looks good in my testing.

bobnik commented 2 years ago

This is a cool feature and it works perfectly. I think, however, that the way it's presented to the user is a bit clunky. It's not obvious that we will be outputting a second file.

Even though a PNG is not really a machine-printable output, I think that the toggle should be removed and the new format should be added to the existing drop list, e.g., "PNG preview". @geekuillaume, if you'd like help with this, let me know.

bobnik commented 2 years ago

One more minor nitpick: We omit semicolons at the end of each JS line as a style choice.

geekuillaume commented 2 years ago

This is a cool feature and it works perfectly. I think, however, that the way it's presented to the user is a bit clunky. It's not obvious that we will be outputting a second file.

Even though a PNG is not really a machine-printable output, I think that the toggle should be removed and the new format should be added to the existing drop list, e.g., "PNG preview". @geekuillaume, if you'd like help with this, let me know.

The goal of this feature is to quickly get a preview of a file to easily show what the generated gcode will draw. It's already possible to export the path to SVG and use this for the preview but this requires more user actions to select the correct format and save it again.

We can change the label of the option to better explain this but I don't think adding PNG as an export format will best respond to the need for this feature.

For the code styling, I can change this but we should force this with a linter executed in github actions to make sure every PR matches the code style used here.

geekuillaume commented 2 years ago

In fact, I just saw that we have an eslint configuration that ignore semicolons to the end of each line. Should we activate the rule to remove semicolons? Also, the configuration is broken because some packages are missing: @typescript-eslint/eslint-plugin, @typescript-eslint/parser and typescript.

bobnik commented 2 years ago

The goal of this feature is to quickly get a preview of a file to easily show what the generated gcode will draw. It's already possible to export the path to SVG and use this for the preview but this requires more user actions to select the correct format and save it again.

Ok. Can you help me understand how the PNG preview provides a better indication of what will be drawn than the preview window itself? I don't fully understand your use case. At one point we had the ability to deselect the current layer, which may be closer to the actual drawn pattern. We could add that back.

For the code styling, I can change this but we should force this with a linter executed in github actions to make sure every PR matches the code style used here.

Makes sense. I can add a PR to fix this and add the rule. Lint was broken when I ported it over to the Vite build.

jeffeb3 commented 2 years ago

I can see both arguments here. I think the intent is to save a png image when you make your pattern, so you can preview the file on a hard disk later (compared to the preview in sandify). I would really like that feature. We just haven't done it before because we didn't have a good way to make it happen. It will be more important with a database feature to share patterns, because you will want the interface to have a preview available.

I don't see a solution that doesn't have negatives.

The png checkmark solution here downloads two files with one click, which I think breaks on chrome. I was downloading a bunch of files without changing the name. Firefox was automatically numbering them for me (sandify.gcode, sandify(1).gcode, etc.). When I added the png, the names got out of sync (sandify(1).gcode was downloaded at the same time as sandify.png). That's not a huge issue, but it is a bit surprising.

Making png a separate output type doesn't really fit the pattern either though. SVG is really for plotters, thr for sisyphus tables and gcode for marlin/grbl machines. PNG isn't for a machine, and all three types of machine could want to keep that preview with the pattern to be able to identify it later. SVG could maybe be stretched to fit that need. We can make the svg settings better, including changing it from black, if that helps.

We could zip up the pattern and the png. I have a personal bias against the zip format (and tar files aren't something most people use), but that would keep the names consistent, keep the files together, and only start one download to make chrome happy. But then you have to unzip the file.

Prusa slicer can embed a thumbnail in the gcode itself. But you can't open it in an image viewer. There is an octoprint plugin to read the thumbnail from uploaded gcode, which is pretty neat: https://plugins.octoprint.org/plugins/prusaslicerthumbnails/ . It's very limited on what can read those files though.

It has also been suggested that we should use the metadata in a png, jpeg, or svg to save sandify project information. If the entire state was saved in the image, then users could "save as..." their sandify project, which would look like an image to any image viewer, but could be imported back into sandify to restore all the sandify settings. The user would then save the project file as a PNG, and then also export the gcode/thr/svg. This is more work, and we have to decide on a save format. This would be a completely different export from the current ones we have. I have a bit of worry that people will publish their image to google photos or open it in an image editor, and it will mangle the metadata.

It seems like it should be easy, but it is hard to couple an image that can be opened on any computer with arbitrary sandify data. We have to make a compromise somewhere. But I would rather try something than do nothing. PRs don't require perfection, just improvement. Sandify isn't finished, so I am ok with moving this forward any way we agree to, and assuming we will have to revisit it in the future, or live with the compromise.

bobnik commented 2 years ago

@jeffeb3, I'll let @geekuillaume pitch in on this, but I didn't get the impression this was a "save once for later" action. It was a frequent action "to quickly get a preview of a file to easily show what the generated gcode will draw". To me, if it's an infrequent action, then the format (dropdown) approach makes more sense. If it's a frequent action that makes changing settings impractical, we should try to use the existing preview if possible to render the faithful representation.

Also not a fan of a zip file for the reasons you put forth.

jeffeb3 commented 2 years ago

we should try to use the existing preview if possible to render the faithful representation

I'm not sure what you mean here. Isn't the existing preview being used to generate the image for the PNG in this PR?

bobnik commented 2 years ago

we should try to use the existing preview if possible to render the faithful representation

I'm not sure what you mean here. Isn't the existing preview being used to generate the image for the PNG in this PR?

Earlier in the comments, I had asked @geekuillaume why the preview did not suffice as a PNG alternative for seeing what would be drawn.

jeffeb3 commented 2 years ago

The preview won't work if you already have the gcode/thr file on your hard drive and aren't actively working on it.

bobnik commented 11 months ago

This PR is no longer compatible with our code base.