evil-mad / EggBot

Software for The Original EggBot
GNU General Public License v3.0
288 stars 140 forks source link

Reduce number of pen lifts in hatch fill #36

Closed ShelMi closed 8 years ago

ShelMi commented 8 years ago

A drawback of the current hatch fill is that it takes a very long time for the plot to complete if there are a lot of hatch lines. If some of the pen lifts could be avoided, the hatch fill would be substantially more usable.

I have been working on this problem, and believe I'm getting very close to a real improvement in plotting speed. The only problem is, my coding style is not up-to-snuff. So...question: is there a coding standards document to which I can refer?

oskay commented 8 years ago

Yes, it is quite inefficient in the pen-lift sense. I have thought of addressing that a few times, and would welcome any improvement.

We do not have a particular coding standard that we require (I suspect that my own coding is not up to snuff, by many standards!) but a good starter guide is here: http://docs.python-guide.org/en/latest/writing/style/

Aside: At a slightly higher level, one might consider using a better filling method altogether. For example, the smooth curves produced by the Hatches (rough) live path effect (detailed at http://wiki.evilmadscientist.com/Creating_filled_regions#Hatch_Method ) actually print much better and faster, because the pen does not have to come to a full stop before turning around. (Another example of this is the one being implemented in the RoboPaint 2.0.0 beta-- you can see a preview in the settings window here: https://github.com/evil-mad/robopaint/issues/232#issuecomment-144303295) Similar fast-path filling algorithms have been implemented elsewhere (including in python) for use with extrusion-type 3D printers, which essentially do this same thing with molten plastic, rather than ink. I wonder if it might not be straightforward to implement one of those?

ShelMi commented 8 years ago

Thank you for the pointer to the style document. I shall read it carefully.

Regarding your aside, I suspect the existing fill method actually is perfectly suited for the task. And it's a very nice piece of work. I have (almost) finished my coding for the sharp-corners method, and in my two test cases I have found plotting speed improvements on the order of 50% - 80%. With luck, tonight I shall be rounding-off the sharp corners, so we'll see what you think then.

If you have any test files you think I should run, please let me know - thanks!

oskay commented 8 years ago

Yes, rounding off the sharp corners could make a very nice improvement. :)

ShelMi commented 8 years ago

Here is an early version, warts and all, for comment. I would particularly appreciate a comment on whether the amount of smoothing I've put on is enough to be kind to the eggbot. Other comments welcome of course.

There are four known issues:

  1. Sometimes leaps across narrow gaps Status: work in progress
  2. Curves extend beyond boundary Status: user can work-around by insetting path, but this doesn't get rid of the fact that joined segments protrude further than unjoined.
    1. Misses obvious chances for joins when ends too far apart.
  3. Roundness of endcaps is not a function of hatch spacing. Status: work in progress

eggbot_hatch_NEW.zip Hatch join hasten test.zip

oskay commented 8 years ago

This looks like a potentially very nice improvement. I really like the curved ends! These should print very efficiently! Let me make a few suggestions, even before looking at the code.

General comments:

(1.) Rather than posting a ZIP here, it is generally considered best practice to fork the repository (I see that you've already done that), open a new branch for your changes, and then (when ready) open a pull request. You can read more about that process here: https://guides.github.com/activities/contributing-to-open-source/#contributing

For discussing the merits of the proposed changes in general, an issue like this is a great method. The comment thread on a pull request is often a great way to discuss minor issues having to do with formatting and integration.

(2.) As this is a natural evolution of the Hatch fill extension, there is no need to create a different extension with a new name. Please go ahead and keep the same name, so that it replaces the older version in the menu. You can give it a version number if that makes you more comfortable. (If you would like to keep it separate during development so that it can be compared to the old version, that's totally fine, but we should converge the names when we pull it in.)

screen shot 2016-01-15 at 1 04 57 pm

The screen shot above shows what the output looks like, compared to the previous version, with the same general settings. (The "reduce pen lifts") option is selected. Some specific comments about the behavior of the extension:

(1.) The hatches should go in front of, not behind, the existing objects. This is a matter of order in the file.

(Getting ahead of our selves a bit, the order of paths in the file should be considered very carefully, as a carefully chosen path order can dramatically reduce the amount of "air time" that the pen spends up, traveling between paths that it will draw. The "reorder paths" extension is one way to do this, but we might consider adding that routine for the hatches that are generated here.)

(2.) If you try to ungroup the hatch paths from the object that they are attached to, you will find that they are quite heavily grouped. Taking the lower left object (hollow star) for example, it took 38 ungroup operations to fully separate the hatches.

What I would hope to see instead is the behavior of the earlier extension: One ungroup operation is necessary to detach the set of hatches from the object, and that the resulting object — separable from the original object — is a single path object, not a group of paths, nor a set of unconnected objects.

(3.) The set of hatches consists of only 38 hatch paths, which means that drawing it takes 76 pen down or up operations This is a big improvement over the 75 hatch pieces in the original version, giving 150 pen up or down movements.

(4.) The rounded edges appear to "color outside the lines" -- which actually doesn't look very good when plotting, coloring in shapes. Can the rounding be added inside the lines instead?

screen shot 2016-01-15 at 1 52 08 pm

(5.) It looks a little haphazard as to which line ends get connected/rounded. It looks as though some lines that have not been could yet be joined, saving many more pen lifts.

ShelMi commented 8 years ago

Thanks for your encouraging words and enlightening comments to this newbie.

I've been having fun working on the program, and have made real progress. But, I haven't gotten any traction with github. I'm totally not "getting" it yet, despite trying tutorials. I'll continue with the git tutorials, but they're so much less fun than working on the program that I couldn't resist popping in again here.

I'm getting reductions of pen up/downs on the order of 75 - 90%. The open star, for example, was: "Saved 87% of 63 pen lifts. pen lifts=8, line segments=63"

Anyway, I want to whet your appetite for my changes by showing the current state: ems star test ems star test hatches

oskay commented 8 years ago

Beautiful! That looks like a huge step up!

And, absolutely no worries on "getting it" -- we're very happy to help, in any way that we can, and we can indeed accept contributions via zip files. If you're interested in going the more traditional "git" route, we would recommend downloading the github desktop app. It's quite possible to use and work with github, even without knowing any of the command line options. (Typically, "clone" a repository to download it with the app, make your changes, "commit" them to create a "save point" and then "sync" to upload.)

ShelMi commented 8 years ago

Yes, I really want to use git. The only version control software with which I'm familiar was Microsoft's Source Safe. The advantages of source control, compared with just uploading .zip files, are overwhelming.

That said, I'd be happy to upload the .zips as they are now if you want them. In fact, now that you mention it, I'll do it right now. I do realize there are many issues left to be addressed - you'll find the issues I know of listed near the top of the .py file.

eggbot_hatch.zip

ShelMi commented 8 years ago

Taking you up on your offer of helping me around my not "getting" the git process, I attach for your comments my latest edits to the hatch extension. So far, the only remaining problem (I think/hope) of note is the one referenced in my comments as issue 19.

[UPDATE: I now have a strong desire to shelve issue 19, so have (3feb2016) stopped work on it.] I am currently working on a fix for 19.

19 Sometimes makes joins that run across a narrow portion that should not be filled, e.g. lower left point of lower left star in EMS test, with 45 degrees, 6.0 spacing, 0 gap, yes crosshatch. Status: Open. Fixing this will increase processing time, as each proposed join will have to be checked against all segments. May or may not be worth either the programming time or the processing time, as can also be remediated by user choice of slightly different spacing.

I have not done a cleanup yet, I'm more interested in the functionality at the moment.

eggbot_hatch 52.zip

oskay commented 8 years ago

This seems to mostly work, up to a few little quirks-- its' obviously a huge improvement over what we have had previously.

What is the issue 19 that you're referring to?
It's not #19 here, possibly this one: https://github.com/evil-mad/EggBot/issues/32 ?

oskay commented 8 years ago

One quirk-- also present in the original, I believe: It does not run if the page size is defined in inches, rather than px.

ShelMi commented 8 years ago

When I referred to issue 19, I guess I was off in my own little world. The number 19 referred to my own list of issues, contained in the comment section near the top of eggbot_hatch.py.

The issue is that when the scope of the segment-joining algorithm is set too high, sometimes a curve can be drawn right across an area that should not be filled in. It is, I think, fixable, but the fix requires a substantial amount of time/work and before I make that investment I would like to see if the problem affects people to a great degree.

Are there additional quirks which you feel merit attention?

I did not know about the page-size-defined-in-inches quirk. My work has piggybacked entirely off the original - and I am almost certainly not competent to fix any issues in the original. I would, however, be glad to at least take a whack at the issue. I do confirm your observation that the original has a problem when the page size is inches.

ShelMi commented 8 years ago

I'm inclined to not even offer the option of doing the connection with straight lines, and just assume the user wants a nice curvy output - what do you think? smooth not optional

oskay commented 8 years ago

On the units: Let's worry about that later. I've been fighting similar issues in other contexts, and should be able to help.

There are some additional quirks-- sometimes not filling certain objects, for example. I haven't found a reliable example to give you yet. I also need to look at the grouping structure a little more. I will get back to you on that.

I'd say go ahead and remove the option; this is a much better default.

Unrelated thing that might be worth considering, if you find that it might be straightforward: When filling in a shape, we often copy and inset that shape first, to make the filling lines not quite go all the way to the edges. When using pens of finite width, that often helps to make a much cleaner shape, since it always "colors within the lines." Since you are already offsetting the straight line segments from the edges in order to allow room for the curves, it might be possible to add an option to inset the fill a little bit from the edges that you are filling.

ShelMi commented 8 years ago

When you find examples of not filling certain objects, it's worthwhile to compare to the identical fill using just the original method. As I say, I totally rely on the original method to do the heavy lifting. This also applies to cases where the grouping isn't right - compare to the original.

I'll remove the option. Should I leave in the SAVE YOUR FILE FIRST message, do you think?

I like the idea of the insetting option. I don't know what worms might crawl out of that can - I think I'll just have to go ahead and try it, and we'll see what problems it does or doesn't make.

ShelMi commented 8 years ago

When I just now tried my test image with inset edges, I ended up with nasty problems in many places. I need to get these problems squared away. It definitely does not look ready for prime time yet, despite my earlier hopes. I'll be working on it.

ShelMi commented 8 years ago

I'm being over-hasty. When I made my comment above, I thought the badness I was seeing was a reflection on the viability of my current version. Turns out it was not a problem with the current version, it was a problem with my attempt at a super-quick implementation of insetting.

ShelMi commented 8 years ago

Hey, wait a minute?? As I've been pondering the insetting problem, I've come upon a question...Have users been advised to do an inset operation before hatch filling? If so, I don't see how that can work properly when there is a shape with another shape inside. I've tried to illustrate what I mean by the attached image, which is of a green original square with interior green square. Selected all. Duplicated. Inset. Stroked red. Original-hatch filled.

In other words, nested objects need to alternately by inset/outset to make this be a good way to constrain the hatch domain of action.

I'm not saying I can't do things right, but I'm sure there will be edge cases where it doesn't behave as desired.

What do you think? Have I just got fog in my cockpit?

inset test

oskay commented 8 years ago

When you find examples of not filling certain objects, it's worthwhile to compare to the identical fill using just the original method.

Yes, of course.

Should I leave in the SAVE YOUR FILE FIRST message, do you think?

No.

I took one of your filled regions, and clipped out the curved ends. Here is the result:

from clipboard

It is neatly inset where the curves were-- just not towards the ends, where the curving breaks down.

For reference, here is what I would consider an "optimal" fill of that region:

screen shot 2016-02-05 at 2 22 26 am

oskay commented 8 years ago

In other words, nested objects need to alternately by inset/outset to make this be a good way to constrain the hatch domain of action.

I don't think that this is the case. If you have a single path object (the kind that can be filled), then the Inset operation should enlarge one boundary and shrink the other.

Example, green original, black inset:

from clipboard

The advantage of insetting (in practice) is that when you draw the resulting shape with a pen, the fill is less likely to bump outside the outline:

screen shot 2016-02-05 at 2 47 35 am

ShelMi commented 8 years ago

eggbot_hatch rev 65.zip You can see that I have added a few options since we last communicated:

  1. Hold back hatch from edges is the feature you suggested. It has several situations where it intrudes further into the border than it should, and at least one situation where it leaves too much room. Details if you want them. On balance, I'm pretty pleased with how my simple-minded algorithm works.
  2. Hold back distance is added because, in conjunction with the new "width of displayed stroke", the user can semi-realistically view and play with the plotted results.
  3. Allowing simulation of the resulting print, by adjusting displayed stroke width, may give useful feedback. At least I liked it. rev 65 example

I was able to come pretty close to your optimal fill of a circle. Had to play with the "scope of reduced pen lifts" parameter a bit to make it connect to the outer hatches. Haven't tried it on any random circles, I expect it won't always be so optimal. rev 65 example circle

oskay commented 8 years ago

That's looking really nice-- I haven't had a chance to try it yet, but I will do so shortly, and hopefully have a fix so that it works across multiple units as well.

ShelMi commented 8 years ago

What is the issue with multiple units?

oskay commented 8 years ago

(Page units of inches and so forth.)

ShelMi commented 8 years ago

Oh, good. In the meantime I'm working on what I hope is the last important problem of the holdback algorithm remaining, illustrated by the deliberately pathological case below:

internal issue 22

oskay commented 8 years ago

I'm attaching a revised version, with some changes-- mostly cosmetic. I've made changes to the description and option names in the GUI.

I would like to suggest that the line width option be removed, and to instead use whatever the parent object's line width is.

I've added an updated "getLength" command that handles other types of units. I have not yet found the issue that is preventing it from working with pages in inch units. Will keep looking.

The "inset" (formerly, "hold back") works reasonably well. It would be very nice if it could hold back from edges that it encounters in the perpendicular direction as well-- see attached image. However, this sounds harder to accomplish. ;)

screen shot 2016-02-12 at 12 50 36 pm

hatch-revised.zip

oskay commented 8 years ago

It seems to be working on inch-sized pages (as of that last version), although inconsistently. Trying to nail this down a little further.

ShelMi commented 8 years ago

I look forward to merging your changes. Do you have a preferred merge facility, or shall I do it manually? [Edit: OK, I've merged them]

Wow - that looks terrible! I have been working on something which will fix the gaps in the hatch, but the current presentation looks truly raggedy. [Edit: Thanks for the pathological fills .svg - should prove useful.]

As far as insetting from perpendiculars, I believe it may be doable. I had been hoping to avoid it, thinking that the user will likely only be insetting enough to avoid any little hooplas ouside his border - sort of a half-pen-width inset. And thus a single hatch too close, but parallel to, a boundary would likely be acceptable.

I'd like to continue working on the fix which accounts for the gaps, before switching my attention to the perpendicular inset problem. That's assuming you think from your knowledge of how people use this that it (perpendicular inset) needs to be fixed?

Speaking of things working inconsistently, I'm rapidly coming to the conclusion that inkex.errormsg has a limited buffer space, and that writing too much debug stuff out causes Inkscape to take a journey into hyperspace. Do you find this to be true, or have I misled myself?

Good luck on the size issue. When does it happen that people use inches as their units?

oskay commented 8 years ago

Without any doubt, the basic "perpendicular" offset that you've implemented thus far is a huge improvement. Detecting the other edge would be even better, of course-- but yes, it's absolutely a huge improvement thus far.

I have not had any issues with large inkex.errormsg outputs, except that they are hard to read. I'm attaching an example of a debugging extension (not guaranteed to run!) that writes a log file to your home directory. If you look at how this one works, you'll see that this method is much more suitable for large quantities of debugging data.

Found an odd issue when filling on a page with inch size units. Screenshots attached. This is on a page with the Letter (8.5 x 11 inch) template, and a simple 5-inch rectangle. Works differently with the option checked, as you will see.

screen shot 2016-02-12 at 10 21 32 pm screen shot 2016-02-12 at 10 22 09 pm

Archive.zip

ShelMi commented 8 years ago

Huh, I'm confused - the first hatch fill of the two above looks like the "hatch (rough)" method - the hatch lines are not straight, and the joins at the end are pointier. How can this be???

oskay commented 8 years ago

I'd advise you to try it yourself, and see if you can replicate it under the conditions that I described.

ShelMi commented 8 years ago

I still don't "get" it. 8^) try replicate hatch problem on 8_5x11

oskay commented 8 years ago

Interesting! I wonder if we're off by a few details in our respective codes.

ShelMi commented 8 years ago

Attached files are where I currently am - I think maybe I've finally licked the "partial blank hatches" problem.

Bug reports appreciated.

Regarding the "odd issue", I cannot think of any way for my algorithm to generate output that looks like the "hatch (rough)" method. I sure hope it's just some kind of mixup. Can you still replicate the results? eggbot_hatch 66.zip

oskay commented 8 years ago

Same behavior with v 66-- I'll follow up later.

oskay commented 8 years ago

Of note: If I start with a generic new document, and then change the size (using document size) to 8.5 x 11 inches (or click letter there), it does not cause the issue. It is only when starting from a fresh new document from Templates > Letter.

ShelMi commented 8 years ago

OK, using the "starting afresh" method, it now reproduces here.

ShelMi commented 8 years ago

Wow - the thing that looks just like the "hatch (rough)" turns out to be the bezier control points being displaced a long way beyond what they should be displaced. Is there any way that the work you've been doing on dimensions somehow does something different to bezier control points, which are in the path as an element of designator type "c"?

[Edit: Oh, no, that's right - I'm not using anything with your dimension work anyway, so scratch that idea.]

ShelMi commented 8 years ago

OK, thanks to your "of note", herewith iteration n+1

QOTD: "So that's what transforms do."

I have not implemented the "other direction" inset, and it may be quite a while before I feel I could do a good job on it.

I hope the attached survives the evil mad scientist torture better than the previous version did! 8^)

eggbot_hatch 67.zip

ShelMi commented 8 years ago

I have become aware of a second, probably less important, issue which can show up when insetting the hatch. Best illustrated by the two images below.

This first image shows the issue behaving badly: test 11 mingap 3

And this second image shows the issue not happening, because min gap is set low enough to avoid the part of the logic which caused the problem shown in the first image: test 11 mingap 1

And here, I think, is the fix for this issue: eggbot_hatch 68.zip

This did lead me to wonder whether the minimum gap parameter is needed anymore, now that small gaps don't have to carry the same plotting-time penalty as they did before. Just wondering, as I do not know what the original need was, that the min gap addressed.

oskay commented 8 years ago

You have nailed that-- the small gap option was a stopgap measure added to reduce the number of pen lifts. Under the circumstances, I would advocate removing it completely. Looks like it might be a good fix for that issue.

ShelMi commented 8 years ago

A "stopgap" measure indeed - perfect definition!!

So...does that mean I am mandated to stripping the gap logic completely out? (As noted above, I think I have already successfully exorcised this latest gap-related issue.)

oskay commented 8 years ago

Yes-- pun not intended-- eek!

Yes, please strip out that logic entirely. :)

ShelMi commented 8 years ago

Bye bye minimum gap. eggbot_hatch 69.zip

ShelMi commented 8 years ago

Found a bug that I think probably predates my messing around with hatch fill (though I haven't actually checked that it does): If you attempt a hatch fill on either an empty document, or on a selection which is not closed, then Inkscape spends forever spinning its wheels while doing and saying nothing. User loses any unsaved work and must forcibly end Inkscape. Proposed fix attached. eggbot_hatch 70.zip

oskay commented 8 years ago

That sounds like a serious bug indeed-- and it wouldn't surprise me entirely, either. It's just the kind of thing that might not actually get tested.

When trying the crosshatch option, on some basic shapes, I was able to get the following error:

Traceback (most recent call last):
  File "eggbot_hatch.py", line 1731, in <module>
    e.affect()
  File "/Applications/Inkscape.app/Contents/Resources/share/inkscape/extensions/inkex.py", line 268, in affect
    self.effect()
  File "eggbot_hatch.py", line 1178, in effect
    float( self.options.hatchSpacing ), False )
  File "eggbot_hatch.py", line 1098, in makeHatchGrid
    retValue = bBoundingBoxExists
UnboundLocalError: local variable 'bBoundingBoxExists' referenced before assignment
ShelMi commented 8 years ago

Thanks for finding that so quickly!

This may fix it:

eggbot_hatch 71.zip

ShelMi commented 8 years ago

I'll start by acknowledging that the way I use Inkscape to design a print may be atypical. When I do a design using hatch fills I repeatedly find myself doing first the hatch fill, then selecting the black hatch and changing its color to approximate the color of the pen I'm planning on using for that fill. (I use a swatch palette that embodies the colors of the pens I have.)

So, from my perspective I think I would like to have the option of specifying that the displayed hatch color be the same as the boundary color that is being filled, rather than always being black.

Does this make sense to you? I've already experimentally made such a change for my own use, but have not done any designing since to see how I like it in practice. Any thoughts?

oskay commented 8 years ago

This is all looking quite nice. Still some slightly imperfect behavior when filling unusual shapes, but it's really quite an upgrade in usability.

screen shot 2016-02-24 at 9 51 24 am

On the fill color/width/etc: I understand where you are coming from. My instinct is that we should pick exactly one behavior, rather than give any UI options. Inkscape has a huge range of line styles, and we can only add an interface to very few of them in the space given.

Here are two approaches that might be worth consideration:

ShelMi commented 8 years ago

Still some slightly imperfect behavior when filling unusual shapes, but it's really quite an upgrade in usability.

Your example looks like an issue that I thought I had fixed. I'll have to get back on it.

On the fill color/width/etc: I understand where you are coming from. My instinct is that we should pick exactly one behavior, rather than give any UI options. Inkscape has a huge range of line styles, and we can only add an interface to very few of them in the space given.

And I strongly believe in following instinct, especially the instinct of a person with such a broad understanding of the customer base.

My preference of your two suggested approaches is the first one. Regarding the second, I would suggest that turning a fill into a hatch might encourage users to be a little sloppy - because of the visual similarity between a filled area and a hatched area and users might therefore get an unwelcome printing surprise when they forget to do a hatch fill. Do you have a preference between your two approaches?

So...what I'd like to do is:

  1. Try to smack down the bug that gave the bad result in your example. I know there will be plenty of other imperfections, but I'd like to at least get out any where I think there is a decent solution.
  2. Try to implement your approach of inheriting stroke options from the parent object. This will be an uphill struggle for me, as I do not yet rise even to the "duffer" level in my understanding of python or of SVG. I welcome the challenge however, it's just that my coding is likely to be very awkward. If that's OK with you, it's OK with me.