Pattern-Projector / pattern-projector

https://www.patternprojector.com/
MIT License
83 stars 17 forks source link

Save stitched file proof of concept #259

Closed cfcurtis closed 3 months ago

cfcurtis commented 4 months ago

Here we go, preliminary proof of concept for saving the stitched PDF!

It's super rough around the edges, and please feel free to suggest improvements or fix anything. If it isn't obvious, I have never worked with JS or React before so I'm definitely just blindly copying things and tweaking to suit my needs.

It does the following

Possible show-stoppers

Other random things that could be improved

vercel[bot] commented 4 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
pattern-projector ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 29, 2024 10:04pm
courtneypattison commented 4 months ago

It's super rough around the edges, and please feel free to suggest improvements or fix anything. If it isn't obvious, I have never worked with JS or React before so I'm definitely just blindly copying things and tweaking to suit my needs.

It does not feel rough at all! This is an excellent PR. Also, it reads like you've worked with JS and React before!

Possible show-stoppers

  • File sizes sometimes get hugely inflated (one went from from 468 kB to 7 MB!)

Definitely not ideal. Was PP able to open the 7MB pdf?

  • Layer names sometimes get completely garbled - it looks like they contain bits of PDF syntax plus bad encoding? Here's an example screenshot: image Other times, the layers are preserved perfectly. I suspect this will need to be fixed on the pdf-lib side, which I probably can't commit to right now. However, I'll try to replicate it with a non-proprietary PDF to at least submit a bug report.

I wasn't able to replicate this issue with my small test set of PDFs. Did it happen with a few patterns or just a couple of outliers?

  • While editing the content stream to do line thickness and remove layers is probably feasible, I'm not sure it would be straightforward with this library, and likely very slow.

Line thickness changing would likely conflict with how PP does it, so I think if we want to keep that on the roadmap, we should plan on pulling the stitching out of PP and into its own PWA. Removing layers would be nice for tablet users who can't use pdfstitcher, so it would make sense to prioritize that feature.

From my end, I'm thinking add save stitch settings to localStorage so at least people don't have to input the stitch settings every time they open the same PDF on the same device. Change the button name to "Export PDF" or even more specific with "Export stitched PDF". Maybe add a tooltip that says its experimental or something like that. Then merge this PR!

Thank you so much for this!

cfcurtis commented 4 months ago

It does not feel rough at all! This is an excellent PR. Also, it reads like you've worked with JS and React before!

Haha, thanks! It's definitely got some oddities that will take my C++/Python brain a while to get used to (like what do you mean I can't assign null to any object??).

Definitely not ideal. Was PP able to open the 7MB pdf?

Great question! I just tried and it looks like the answer is no, so that's a pretty big problem.

I wasn't able to replicate this issue with my small test set of PDFs. Did it happen with a few patterns or just a couple of outliers?

It happened with 2/6 that I was testing with. However, I just tried now on my work computer running Windows (instead of Linux), and one of the files did not have the same issue when opened in Adobe Reader, but the other just said the file is damaged. That one was an older Jalie pattern if you have one kicking around to try.

Line thickness changing would likely conflict with how PP does it, so I think if we want to keep that on the roadmap, we should plan on pulling the stitching out of PP and into its own PWA. Removing layers would be nice for tablet users who can't use pdfstitcher, so it would make sense to prioritize that feature.

I agree, it's nice having two different approaches to line thickness. For removing layers, I think it's doable to turn on/off the layers with this API and that should work on a tablet, but the thing that PDFStitcher does to actually remove content is very computationally intensive. I mainly put it in there so that you could isolate a size and then edit it in Inkscape, but Inkscape now imports layers properly so perhaps it's a feature that can fade into oblivion.

From my end, I'm thinking add save stitch settings to localStorage so at least people don't have to input the stitch settings every time they open the same PDF on the same device. Change the button name to "Export PDF" or even more specific with "Export stitched PDF". Maybe add a tooltip that says its experimental or something like that. Then merge this PR!

Saving stitch settings for a specific file makes a lot of sense! I'm totally fine with any UI or implementation changes you want to make, and an "experimental" tooltip seems appropriate given the issues with a limited test set.

Thank you so much for this!

Thank you, it was fun to learn something new!

courtneypattison commented 4 months ago

For removing layers, I think it's doable to turn on/off the layers with this API and that should work on a tablet, but the thing that PDFStitcher does to actually remove content is very computationally intensive. I mainly put it in there so that you could isolate a size and then edit it in Inkscape, but Inkscape now imports layers properly so perhaps it's a feature that can fade into oblivion.

I really like the idea of turning off the layers rather than ripping them out if that's feasible!

Even with it's issues, I think we should add it in and try to improve it when we have time. It's better than no export option and with a little love it could turn into something great!

sashasewist commented 4 months ago

So impressive!

sashasewist commented 4 months ago

If the save PDF with only selected layers visible could be implemented the enormous use case would be people printing patterns from mobile. Right now the only app that can save selected layers is AD on ipad, but learning curve is huge. And all other devices have no way of printing layers or saving PDF with selected layers. It would be AMAZING, and you would become the biggest star in the PDF pattern printing community!! You'd have the PDF pattern triple crown! Loved by projector users, ditto users, and printing users! 👑 👑 👑

sashasewist commented 4 months ago

Which encrypted patterns created problems? So far, all of the encrypted patterns I have tried have worked fine.

sashasewist commented 4 months ago

One issue I have found in my tests, regardless if the pattern is encrypted or not, is that while layers are showing up in layers panel in the saved PDF, they don't actually work. All sizes visible no matter what is selected. Free patterns from Buttons and Blueprints, Sinclair, Sew a little seam, P4P, all tested and had this problem.

cfcurtis commented 4 months ago

Thanks for testing! It's definitely got a ways to go but I have hopes that we can iron out these issues.

  • Jalie has all projector files now, no one will be stitching their old files, so ignore problems with their PDFs.
  • What has the other pattern that had the layer name issue?
  • Which pattern had the big increase in size

The "layer name issue" is actually a corrupted file, but Evince on Linux was happy to open and try to display it with messed up layer names. When I tried on Windows Adobe would not open the file at all, and PP also struggled.

I mentioned Jalie just because it's a popular brand that others might have for testing. The other one is a Sunny Mountain pattern that Cathleen sent me for testing purposes, I can ask her if I can share it! All files have increased in size in my tests, but the Sunny Mountain one was the 14x increase. I also tried a Stitch Upon a Time pattern (Midnight Slippers) where only the last 4 pages were pattern pieces, and the file still went from 8.5 to 10 MB.

One issue I have found in my tests, regardless if the pattern is encrypted or not, is that while layers are showing up in layers panel in the saved PDF, they don't actually work. All sizes visible no matter what is selected. Free patterns from Buttons and Blueprints, Sinclair, Sew a little seam, P4P, all tested and had this problem.

I'd categorize this as a show-stopper as well! I thought it was working for me in Evince, but that must have been the previous commit where I was editing a copy of the file instead of creating a new one. I think I'm missing something in copying over the layer info and it's causing all sorts of issues.

Ultimately I'm not sure that this pdf library is ready for prime time, but hopefully we can make it happen. I'm about to head out camping for the weekend, but I can make some time next week to revisit this.

sashasewist commented 4 months ago

Have fun camping! ⛺️♥️😁

courtneypattison commented 4 months ago

@sashasewist thank you for the thorough testing! I'm going to hold off on adding this PR to this release until/if we can get the layers working. @cfcurtis please let me know if you would like my help with any of these issues when you get a chance to work on this again. I'm focused on this release right now but I should have some more time next week. That being said, I'm still learning about PDF so I'm not sure how helpful I'd be!

cfcurtis commented 4 months ago

Okay, I snuck a bit of time on this and I don't have a solution yet but I think I know the problem so I'm writing it down here so I don't forget it!

I'm copying the values from the document catalog, but they're just PDF references (e.g. 123 0 R). The associated objects are not being copied. I managed to see the referenced object with inDoc.context.lookup(value as PDFRef), but I haven't yet figured out how to actually copy it over to the target document (I tried outDoc.context.assign(value as PDFRef, copier.copy(xObject)) but this causes an error in saving the document. However, I think it'll be something along these lines.

Edit: I just saw @courtneypattison's comment, I agree that we should hold off on merging this! I'll pick it up again Monday or Tuesday, but feel free to play around in the meantime.

cfcurtis commented 4 months ago

Unfortunately, I think I'm going to have to close this PR for now. I tried a few different approaches to get things working, but ultimately I'm still getting corrupted files and non-working OCGs.

The problem with pdf-lib's built-in page embed method is that it creates a copy of all the resources, including references to OCGs. This means that the reference numbers are wrong, so the OCGs don't work at all. It also does some kind of "normalization" on the pages, which I believe is where the inflated file sizes come from.

I tried rolling my own "Page to XObject" function and writing the content stream directly (this is how PDFStitcher does it), but for some reason the resulting document is blank, so I'm missing something somewhere.

Unfortunately I'm running out of time for working on this! I'm not sure when I'll be able to revisit it but I would be totally happy if someone wants to pick it up in the meantime.

cfcurtis commented 4 months ago

Just kidding! I realized I was defining the XObject stream contents incorrectly so I fixed that, and now some of my test files are behaving, layers and all. The file size inflation part is fixed as well.

Remaining issues and things to do:

sashasewist commented 4 months ago

OMG Amazing!!!!!!! I just stitched a dozen different files and they all worked perfectly!! Layers preserved and work as normal! 💯 🥇 😍

courtneypattison commented 4 months ago

This PR is such a whirlwind! You're a freaking whiz @cfcurtis! You've resurrected pdf-lib!

A Wardrobe By Me pattern that I tried to open silently failed when I pressed the save button.

This feature is really shaping up! I think your checklist is very reasonable. I'm not sure what state you want this feature to be in before merging. I'm happy to merge it into beta whenever you would like!

sashasewist commented 4 months ago

A Wardrobe By Me pattern that I tried to open silently failed when I pressed the save button

Which browser? I just did WBM urban sweatpants file in Firefox on windows and it worked fine. BUT that file brings up something that could be added to roadmap for this feature: page layout as columns instead of rows. The pages in this file need to be laid out as columns (page 2 below page 1, not to the right of page 1). PP layout only allow rows based layout. This is not just needed for save feature but for PP stitching/viewing as well.

courtneypattison commented 4 months ago

Chrome, but I just tried in Firefox and it's also not working there as well. It's the Wanda Wrap Dress. I could mangle the PDF if you'd like a copy @cfcurtis.

@cfcurtis mentioned before that putting options like that in an advanced menu might make sense, so we can hide complexity that most people won't need to use. I like that idea but I'm not sure where to put the menu!

sashasewist commented 4 months ago

Yes that makes sense to hide it. But I also just remembered the workaround of changing the page range: 1,10,2,11, etc fixes it for now.

sashasewist commented 4 months ago

One plan to consider for an advanced menu options, would be to fork a completely different app called something like pdfstitcher.online But not sure if that brings disadvanges to be separate from PP. I'm thinking something like pdfstitcher.online could just have the save PDF functionality, with the functions that are not as necessary for PP:

Or alternative plan if it's better to keep this all in PP:

cfcurtis commented 4 months ago

This PR is such a whirlwind! You're a freaking whiz @cfcurtis! You've resurrected pdf-lib!

Haha, thanks! I definitely gave up and then realized the problem.

A Wardrobe By Me pattern that I tried to open silently failed when I pressed the save button.

Did it log any errors? Right now I have it just throwing an error when an array of content streams is encountered.

This feature is really shaping up! I think your checklist is very reasonable. I'm not sure what state you want this feature to be in before merging. I'm happy to merge it into beta whenever you would like!

I feel it could be merged to beta soon, and then people can inform us when the issues inevitably happen (beyond what's know). I like your idea to clearly flag it as an experimental feature.

@sashasewist I do think there's room for a hidden "advanced stitching" menu, but unfortunately removing layers and editing line properties is quite complex and slow. I just don't thinking it's practical in a JavaScript only implementation right now. However, disabling layers so they don't show up by default should be possible, so if that's useful as a "separate from stitching" feature then hopefully it can be done!

courtneypattison commented 4 months ago

@cfcurtis you're right, there was an error about content streams not being supported!

So many options @sashasewist lol! I think having stitching be a separate app makes the most sense long term. That being said, right now PP has a harder time with stitched together PDFs so it's better for people not to export PDFs and instead just stitch without exporting. The reason is that we have to downsample larger PDFs to not hit browser limits. I've looked into tiling larger PDFs in #245 but it's not supported by pdfjs and results in worse performance. Always tradeoffs! I'm hoping to resolve this issue at some point but it's a tricky problem.

Another consideration is that we can't pass PDF data easily between routes e.g. patternprojector.com/calibrate and patternprojector.com/stitch. That's the reason why I put projecting and calibrating in the same route.

@cfcurtis what do you think about where to put the stitching? I think the main concern is how much time/effort do we want to invest in an online version of pdfstitcher? I'm feeling like PP is already taking up too much of my time; I should be doing more job hunting! So I'm not sure how much time I can volunteer. I really wish I had more time.

cfcurtis commented 4 months ago

@cfcurtis what do you think about where to put the stitching? I think the main concern is how much time/effort do we want to invest in an online version of pdfstitcher? I'm feeling like PP is already taking up too much of my time; I should be doing more job hunting! So I'm not sure how much time I can volunteer. I really wish I had more time.

I too wish I had more time! I haven't even started on my main research project for the summer yet (pattern piece identification/normalization for easier editing). I would say that I can likely implement the things on my checklist, but I would draw the line at actual content stream changes like line properties or removing layers.

If saving PDFs with layer selection preferences is a priority, then I'd say we should move the "save PDF" button outside of the stitching menu, as it's confusing for that to only show up when there's a multi-page document. Regarding "advanced" features, I think the easiest thing right now would be to direct people to download PDFStitcher. I'm also partway through a refactor that would make a server side implementation easier, but I get nervous about PDF usage agreements with server side processing.

Good luck on the job hunt, that's certainly a full time job! Hopefully PP gives you a good portfolio piece that people appreciate.

sashasewist commented 4 months ago

Such helpful info for me to understand the context of everything, and tradeoffs! Courtney, definitely don't feel any guilt about starting to put PP on backburner, you have already made miracles and solved so many problems, you can be proud and satisfied at what a good app you have made, even if further updates can't happen!

courtneypattison commented 4 months ago

Thanks team :) Let’s keep it simple then. @cfcurtis let me know when you think this PR is good enough to merge and I’ll add some small UI changes like the tooltip and maybe a pop up on error. I’m not sure where we’d put the button since there’s so little room in the main menu. I’m thinking make the stitch menu icon always available. Maybe combine the stitch and layers menu into something like a layout menu and have the export in there since this feature makes sense only for multi page or multi layer PDFs.

cfcurtis commented 4 months ago

@courtneypattison Sounds good! I'll see what I can do in the next couple of days.

I agree that there's very little room already, I'm not sure where to put the "save" button. I tried testing it on mobile (Chrome on OnePlus 7T) and I could only see the button if I went into desktop mode (and some of the other UI elements are cut off for me):

IMG_20240529_094222

For that matter, I tried saving the stitched PDF from mobile and it's just kind of hanging, so I'm not sure that this is a silver bullet for mobile users :(.

cfcurtis commented 4 months ago

@courtneypattison Okay, I think the quick wins are in place! I broke the save button out into it's own component so it's easier to change the UI. Right now I have it popping up at the bottom left when a document is loaded, independent of the stitching or layer menus. The name of the file still has -stitched appended, which may not make sense anymore, but that's a minor thing.

It now saves user-selected layer visibility and handles arrays of content streams, so I'd say that I'm comfortable merging it to beta for further testing (unless @sashasewist finds some more show-stoppers!). The remaining issues that I know about are:

Finally, I got it to work on mobile with a small test file, but it took a while. Maybe notification or spinner of some sort to let people know that it's not just frozen would be a good UI enhancement.

courtneypattison commented 4 months ago

Awesome @cfcurtis! Thank you :) I’ll try to work on the UI this week!