Adds the ability to export a scaled spritesheet. #300
Conversation
| } else { | ||
| canvas = sourceCanvas; | ||
| } | ||
|
|
There was a problem hiding this comment.
You can reuse ImageResizer.resize to resize the canvas. The resizing should also be moved to the caller (ie PngExportController). CanvasToBlob is a helper and can not assume it is used for exporting.
|
Hi ! (and thanks a lot for contributing !) Let's start with a user setting for now, but the spritesheet export needs to go have a dedicated dialog to take care of scaling and other features such as issues #213 and #247. This dialog would support ZIP, PNG with all the options needed. As you've probably seen, the drawer is nice and all, but you run out of space pretty quickly. Now I'm fine fine with having a temporary solution without the dialog :). I would move this setting to the PNG export section though. I think the feature will be easier to understand this way. I added some comments in the files review, if you want to have a look. Nice work, thanks again :) |
…akes description spans with inline styles into divs. Adds a little more margin below the scaling factor so it's not too snug with the Download PNG button.
|
Thanks for the feedback! Sorry it took me a couple days to have time to sit down with this and update. Always happy to do cleanup on a codebase where needed. :) ImageResizer is definitely a cleaner approach as well. |
…exportscale Conflicts: src/css/settings.css
|
Sorry for the delay ! The change looks good, ready to merge. And thanks for the cleanup :) I'll just test it one last time this weekend. Thanks ! |
|
Looks really good ! Maybe one last thing before merging : I would like the UI to be a bit more compact. Move the Label "Output Scaling Factor" inside the .scaling-factor div, right before the input range. The label's text can be changed to "Scale" to be shorter. This way everything related to scaling will be on the same line. If you want you can add tooltips to any element following the example below : Other than this it's ready to merge. |
| // Output Scaling Factor | ||
| var scalingFactorInput = document.querySelector('.scaling-factor-input'); | ||
| scalingFactorInput.value = pskl.UserSettings.get(pskl.UserSettings.EXPORT_SCALING); | ||
| this.addEventListener(scalingFactorInput, 'change', this.onScalingFactorChange_); |
There was a problem hiding this comment.
It would be nice to have a listener on the 'input' event as well :
this.addEventListener(scalingFactorInput, 'change', this.onScalingFactorChange_);
this.addEventListener(scalingFactorInput, 'input', this.onScalingFactorChange_);On browsers that support the 'input' event properly on range inputs, this will fire everytime the value is updated, so you will see the number change in real time.
There was a problem hiding this comment.
We need to make sure the event listeners added are removed when the panel is closed. This is handled by pskl.controller.settings.AbstractSettingController#destroy . PngExportController and GifExportController both extend AbstractSettingController without overriding the destroy method. All their events are automatically cleaned up.
Here you can either add a call to this.superclass.destroy.call(this); at the end of the destroy method. Or you can move the event's logic to the PngExportController.
There was a problem hiding this comment.
Done, went with the superclass call for now.
|
Added 2 small comments on ImageExportController, would be great if we could also take care of this before merging :) Thanks ! |
|
Sorry for the long delay again, I think I've addressed the last few comments. :) |
|
Looks good to merge ! Thanks a lot for your contribution. For information, I will try to release a new version of Piskel either this week or next week. Your feature will then be available on piskelapp.com and in the new desktop applications. I'll make sure to mention you when I publish the release :) (and I should have a contributors' section somewhere, but not sure I'll get around to do that this week) Thanks again ! |
Adds the ability to export a scaled spritesheet.
This is a patch for the second feature I requested in this tweet: https://twitter.com/lopar/status/637802318008717312
Please let me know if there are any tweaks to the UX or whatnot. For one, I was debating whether the scale should appear as a general export setting or as part of the spritesheet setting section, since it currently only works for spritesheets. I went with general export since it'd be nice to extend it to others types such as ZIP in the future, but I'll defer to whatever you think is best there.
The scaling method with canvasses should work on all modern browsers (IE 11+ and Edge, Chrome, Firefox, Safari, Opera probably).