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

Fix root path for export on krita4.3 #42

Merged
merged 2 commits into from
Jun 2, 2020

Conversation

leiserfg
Copy link
Contributor

No description provided.

@leiserfg
Copy link
Contributor Author

In the way that the name of the root node is replaced by 'export' on the plugin does not work with krita 4.3, that causes that the exports where created in the same folder of the kra file, instead of in a folder. That was not a big issue per se but it also broke the COA exports.
This patch is intended to fix the bug without breaking backward compatibility.

@NathanLovato
Copy link
Contributor

Thanks for your contribution!

Here's an issue remaining:

Screenshot from 2020-05-30 07-59-13

Exports like so:

Screenshot from 2020-05-30 07-58-14

It should all be in export/, and 3.png should be in export/Layer_4/Layer_5/3.png

Here's the file for testing:

test.zip

@NathanLovato
Copy link
Contributor

We should start to keep some test document and maybe have a regression test?

@leiserfg
Copy link
Contributor Author

leiserfg commented May 30, 2020

Now everything is created inside of the export folder, anyway I don't know if everything is working fine with the COA export because I don't know how it should work in that case.
I replaced the use of os.path with pathlib to make the implementation easier to read and to avoid doing manual path separation/joins because it can be problematic on windows and I have no windows to test.

@NathanLovato
Copy link
Contributor

@larpon, do you have a sample krita file we could use to test the exporter with COA tools? A simple file with a real-world character with the correct JSON/export we could keep in the repo for automated testing would be great, that way we can try exporting from new versions of the add-on to COA tools and see if it breaks.

@larpon
Copy link
Contributor

larpon commented Jun 2, 2020

@NathanLovato - yes, sure I'll cook something up

@larpon
Copy link
Contributor

larpon commented Jun 2, 2020

@NathanLovato

Test files: #44
Fix for recent changes: #43

@NathanLovato
Copy link
Contributor

Just tested everything, looking great, thanks much!

@NathanLovato
Copy link
Contributor

I'm merging manually to fix conflicts

@leiserfg
Copy link
Contributor Author

leiserfg commented Jun 2, 2020

IMHO we should avoid using os.path.sep at all because it does not work that well in windows (there both / and \ are valid). That's why I'm using pathlib.

@NathanLovato NathanLovato merged commit 2082395 into GDQuest:master Jun 2, 2020
@NathanLovato
Copy link
Contributor

Merged manually in 2082395

@NathanLovato
Copy link
Contributor

Ah, I hadn't paid attention. @larpon is there any reason you're not using os.path.join? @leiserfg is there something pathlib does that os.path can't handle well?

@NathanLovato
Copy link
Contributor

Forget my previous comments, re-reading the code and thread I see I should've drank some more coffee this morning. I'll clean up the changes. Sorry for the hasty merges!

@NathanLovato
Copy link
Contributor

Done, should be all good now. Thanks for your contribution!

@leiserfg
Copy link
Contributor Author

leiserfg commented Jun 2, 2020

@NathanLovato I have another idea, the last one 😄, we should expose a way for allowing the user to use nearest-neighbor (NN) as interpolation algorithm, most of the time you want smooth interpolation, but for those doing pixel art is better to use NN for up-scaling.

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.

None yet

3 participants