-
Notifications
You must be signed in to change notification settings - Fork 55
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
Break everything in the right way. #104
Conversation
While it's good to have some provocative input for a discussion on the structure of our code and I agree with at least half of your changes, I feel you are missing a key point: Removing features which are useful outside of VisiCut is not helpful in the long term. Imagine some use cases for LibLaserCut:
Most of these will require a number of features that, if I understand correctly, you want to remove from LibLaserCut:
So it's not helpful to remove any of these features from LibLaserCut. If LibLaserCut is reduced to a nice and simple core, but everyone is forced to write their own (duplicate!) code for sorting inside-out, the situation is worse than before. Regarding LaserScript: It makes no sense without LibLaserCut, but there are more uses for it than only VisiCut, for example a command line interpreter. Therefore it should not be moved from LibLaserCut to VisiCut. It should rather be a separate optional package inside the LibLaserCut source tree. As a constructive suggestion: Would it be possible to first limit your changes to everything that does not break features, merge that, and then start a separate new look at what's remaining? For example, just start one branch for cleaning the dithering, create a separate pull request for that, discuss and merge that, and then go on with the next thing. I don't think the current proposal will ever by merged as a whole and it would be sad to just get stuck in disagreement. |
Imagine some use cases for LibLaserCut:
You misunderstand. The problem isn't that LaserScript makes no sense without LibLaserCut. It's that LibLaserCut does makes sense without LaserScript. Clearly you'd have LaserScript with a dependency on LibLaserCut. Since it makes sense without Visicut, it's certainly it's own thing and should be that. It's a program that very clearly shows what LibLaserCut can do. I'm not saying you need to delete laserscript forever, just that it's not something that make LibLaserCut important. |
The problem here is that everything that expects a BlackWhiteRaster is now broken. That's RasterPart and that's several elements within Visicut. It requires replacing all the stuff in dithering because they are now using a different and more common format. Without BlackWhiteRaster, some elements of RasterPart break down. They call some specific functions like isBlack() within that. And some of the optimizations that were specific towards shining that turd have to be adapted into things that slow those things down rather than speed them up. Like ByteArrayList which copied the data into a nice class that could get reused and demanded to be scanlines of the correct size. But, with a BufferedImage I can just hand the bytes that are actually in the picture. I don't need that. But, it's best to leave it because drivers use it and no since touching them at all because that could do spooky things. Which is not to say there are not ways to thread that needle a bit. It would be possible to have the slow pointless dithering algorithms that work with BlackWhiteRasters, and the BufferedImage functions. Then you could just call the functions normally. Then when you've properly finished up. You can iterate over every bit of the BufferedImage and feed it into BlackWhiteRaster. Or better yet, rip out the guts from BlackWhiteRaster and just make it store a BufferedImage for the memory. Switching from bytes to integers and fixing the interleaving didn't cause any issues. The same would be true if you made BlackWhiteRaster a dummy class stored a BufferedImage. You could also do the same for another BufferedImageAdapter class that simply hands the actual colors over because it knows the underlying BufferedImage is actually grey. AbstractLaserProperty can be added without objection. Then it could just replace all the random LaserProperty classes whenever that comes around. Also it would be possible to move the big catch-all raster to vector converter I moved to Rasterable and renamed "toVectorPart()" simply included anyway. Even if you didn't remove the weaker version of the code (though it actually works currently) from LaserCutter. The highly helpful additions could certainly be added in there parallel with some of the more problematic code elements. Most of my design choices are such that the hooks to Visicut are pretty easy to go through. In fact, I don't have it uploaded, but I do totally have a version of Visicut that works with my LibLaserCut version. I need it for bug checking. It does for now have the removed elements of LibLaserCut within the Visicut. |
Actually upon review, some of these certainly could be done piecemeal with approvable selections. It's entirely possible to rebase things on AbstractLaserProperty including the core classes without anything noticing. Then people could consider it a bug that a property was casted to something it wasn't then tried to read the getSpeed() when they could have obviously just done getFloat("speed") on it and not assumed it was one of those classes. Which would then just be 4 lines and some helper functions to adapt to the older code. It would also be possible to include the corrected Libraries for BufferedImage to BufferedImage conversions, along with some dummy modified classes that actually use them. Like rebase BlackWhiteRaster on an actual public BufferedImage that is black and white. And take that as a constructor, but also let it call all that old code to run the other dithers the slow way without utilizing the changes. Building up some scaffolding etc. The expanded actually helpful toVectorPart() function doesn't have any problems anyway. It could just be included. Then the overly tight coupling with Visicut could be piecemealed in switching it over to the new functions. The problem with this though seems to be that at the end deleting all the old code might not be an acceptable thing. And this is generally because I'm waiting around for some shoe to drop with my K40 driver or somebody to say they tried it or anything to happen. Streamlining a library and fixing APIs are an easy thing for me. Big changes certainly don't seem like they'd get approved especially from a repository that gives you the ax for having the wrong type of return characters.
I'm really just waiting on some feedback. I don't know the range of the feedback, from "Oh my god! What am I saying, you are god!" to "I wish I could say that your driver only broke my laser, but I can't because it also burnt down my house and killed my children." I'll finish up the streamlining and break some of it into digestible chunks. |
--
"But, Tatarize, you changed a lot of stuff and more and it won't work with with visicut anymore!"
I consider that a feature. You want a library, stop tying those things together so heavily.
"But, Tatarize, you didn't really optimize that stuff."
Yeah, but now the code could be strongly optimized. I mostly adapted the stuff. Note none of the drivers have changed one iota.
"But, Tatarize, you didn't test everything fully."
That's true, and relevant. Obviously out of the gate it's going to have some bugs. And may present such a list of breaking changes that it almost seems like a non-starter especially for something that might not present any killer feature right off the bat.
Basically,
BlackWhiteRaster
,GreyRaster
,BufferImageAdapter
, are weak, non optimizable BufferedImages . All the raster algorithms got made into a static master class ofDitherFunctions
I added a bunch of ColorFunctions and GreyscaleFunctions rather than the one algorithm that was there, there are now six. (see: http://www.tannerhelland.com/3643/grayscale-image-algorithm-vb6/).More simply you call GreyscaleFunctions to turn a bufferedImage into a grey bufferimage. Then you hand that to the Raster3dPart.
You call DitherFunctions to turn a bufferedImage into a 1 bit buffered image. Then you hand that to RasterPart.
The LaserCutter stuff doesn't really need to care how that stuff gets made either.
The general point is all that stuff that was pretending to be complex stuff was all just BufferImages in disguise, slow, poorly optimized without any memory copying features, bufferimages.
The Dither and Greyscale functions should be moved out of the LibLaserCut package and put in Visicut. Then LibLaserCut's Raster3dPart simply takes a greyscale image and RasterPart takes a 1 bit indexed image (black and white, or for our purposes burn and don't burn). If your job is laser cutting you don't need to have lot of image manipulation algorithms. Absolutely nothing depends on them within the project.
A lot more stuff also didn't matter to the library, that stuff should just be put in Visicut, or a different library. Things like rasterscript depend on LibLaserCut, LibLaserCut does not depend on them.