search

t-oster / LibLaserCut

a platform independant library to control Lasercutters. This is the base library for VisiCut
http://visicut.org
Other
59 stars 55 forks source link

Simplify things for Driver Authors. #106

Open tatarize opened 5 years ago

tatarize commented 5 years ago

I just wrote a driver. And somethings were annoying and wasted my time. Everything really should be setup to help driver authors. You basically need to hit a trifecta of has that particular model of laser, knows the format it uses, internally and is pretty good at java. But, throwing up roadblocks isn't very nice.

A lot of stuff that could have been done for me, was not. When it looked like it had been, it was a lie and I felt bamboozled. Every driver writes their own raster code. Just because the GenericGcode version put it's raster in LaserCutter doesn't mean it doesn't. And it wasn't something I could use.

I had to write my own from scratch. After wasting a while around aimlessly looking through the code that was supposed to help me.

The LaserProperty stuff is dreadful. If I didn't use one of the premade expected to be used classes which apparently have a different version for each property missing, or an option to hide that property etc.

Using native speed units is somehow discouraged. That's actually weird. I could see not having them but the actual speed things travel is a knowable thing on every device and useful information.

Why is IModelaMill.java the only place that thinks to use a Map for that stuff. In my tear-everything-down pull request (still buggy, won't work with Visicut as stuff needs to get moved and some adapters changed #104 ), I wrote AbstractLaserProperty ( https://github.com/tatarize/LibLaserCut/blob/master/src/com/t_oster/liblasercut/AbstractLaserProperty.java ) as a class that basically hooks up the LaserProperty with Map<>. With that you could throw out basically all the generic LaserProperty classes and the special ones in the drivers could be reduced to a couple lines contained within the drivers themselves:

    AbstractLaserProperty prop =  new AbstractLaserProperty();
    prop.addPropertyRanged("speed", 50, 0, 100);
    prop.addPropertyRanged("power", 50, 0, 100);
    prop.addProperty("Auto Focus", true);
    prop.addPropertySpecific("gear", 3, 5, 7, 9);
    this.addPropertyRanged("frequency", 500, 10, 5000);
    return prop;

I was not given clear enough guidance on the units to be used. Nor am I permitted to simply request the correct units be given to me. I needed to find the right helper function somewhere to convert the numbers it gave me in the units I wanted. And do some math there. And I'm not told explicitly whether these are absolute units or relative units. The sample driver told me:

Both of which then quickly converted that to mm, even though the moveto was apparently supposed to be in mm already and somehow different. I get that they are apparently in dots and I wanted it in mils. Better start doing some math.

The fact that speed gives me percents was and still is baffling. I know whatever other lasercutter you use had it in percents. But they don't compare one thing to another thing at all. If you know the maxspeed of those you could convert them.

This on top of the seeming overemphasis on seemingly trying to shoehorn me into using one of the predefined class types. Made it seem like giving my speed in percents was the expected course of action and my filthy discrete units of actually telling me what it's doing might be acceptable if I conformed to that and didn't try to use any of rest of the API.

If my device exported a job as a binary file, I'd be screwed. I'm given a PrintStream so I couldn't really hand it a .mol file or some ruida compatible file. As those need binary exports.

The time estimate isn't done for me. I mean I get that you have speed as percents. But, if you had them as actual speed, it's pretty easy to just give it a solid guess as for the time estimate.

There's a bunch of other stuff in the driver, that doesn't need to be there. Anything that the library doesn't need shouldn't be there. If you can delete a file, it should be deleted. It make it easier to know where I should look. As everything is important.

I couldn't just tell it my device runs at 1000 dpi only. It got weird and started throwing errors. That would have nicely given me the positions in mills, which is what I wanted.

There's no way to provide any extra functionality to the program. I have some other functions like fire the laser, rehome the device, nudge the laser head around. That stuff is pretty important and gets left on the cutting room floor.

tatarize commented 5 years ago

107 should replace the code that raster code that hurt me to look at.

108 should make the properties less of a pain.

109 should simplify making raster code work.

These, and a rewrite of the example driver to show how to utilize the them effectively should suffice. I think. If you already have working code, you should just be able to plug it in. The vector code gave that to me. The rest did not.

t-oster commented 5 years ago

I am on vacation next week, so I won't be able to review.

tatarize commented 5 years ago

Using a AbstractLaserProperty as a simple drop in replacement for vector and raster settings works fine. In the lasercutter settings it changes the structure of the XStream on the visicut side. It doesn't actually seem to care whether or not I'm a LaserProperty or not. Which makes being one a bit of a waste. It just serializes the class up. It seems like a decision should be made for one method or another. Either make it a serialized class or make it a LaserProperty and use that. In any event things continue to work if you just add in a couple places to sync the LaserPropery delegate and the actual values in the lasercutter. But it's more confusing than I'd like.

tatarize commented 5 years ago

Using the RasterBuilder works fine assuming BlackWhiteRaster is backed by a RasterElement class that you can get access to. So that part of my driver rewrite will have to hold off until #107 goes through. I'm trying to balance the fact that Visicut has no methods to stop retrying after the USB cord is permanently unplugged, and the desire to make sure that that doesn't actually kill the project. I got it working but then it worked so well that it would survive turning the device off and back on. And there's no way to tell it to STOP so I'll opt for turn it off for 30 seconds, then I'll throw an error. Providing feedback through the ProgressListener because the warnings don't issue until everything is done.

tatarize commented 4 years ago

It seems like some breaking changes might be preferable at this point. #115 for example is fine. But, I feel like even if I nudge things to be properly untangled the spaghetti code will remain in rather than be excised.

1) Make BlackWhiteRaster implement RasterElement.Provider(), add in another greyraster than just implements GreyscaleRaster and RasterElement.Provider() 2) Add in a graphics library to visicut that can do all the BufferedImage graphics changes. This means all the transitions to grey and all the dithering algorithms. 3) Use the graphics functions in visicut for all the graphics stuff done in the application. Include a function that will convert BufferedImage to RasterImage. 4) In Visicut, make Raster3dPart use BlackWhiteRaster and GreyRaster rather than BufferedImageAdapter. 5) In LibLaserCut, Change Raster3dPart and RasterPart to rely on the image as RasterElement.Provider() rather than as GreyscaleRaster. 6) Abandon BlackWhiteRaster. 7) Replace image (RasterElement.Provider class) in Raster3dPart and RasterPart with RasterElement. 8) Since the underlying datastructure is the same and robust, Raster3dPart and RasterPart can have all of their methods put into the base class since they all actually do the same thing. 9) Replace the functionality of LaserCutter.convertRasterizableToVectorPart() to use RasterBuilder.

And that would be one part of one element and it doesn't remove a lot of the coupling between the library and visicut. Really with the Builder the rasterpart and raster3d part could just be replaced with RasterElement, then drivers could throw into a RasterBuilder. And emit elements. The use of XStreams by Visicut makes the driver properties sort of locked in there, it seralizes the data in the class and really then the drivers don't need to implement LaserProperty. The VectorCommand of sending a LaserProperty element is actually a bit weird. It should just send each 1 property change each command. But, that's API stuff and pretty locked in.

tatarize commented 4 years ago

116 starts to replace some functions with things that should be absolutely identical. I've since thought of a different transition route.

Since the only raster that isn't easily backed by RasterElement is BufferedImageAdapter. I could simply make BufferedImageAdapter implement RasterElement.Provider but rather than giving the backing object it would convert the BufferedImage it's backed by into a RasterElement.

Then at a minimum all the elements would be RasterElement.Provider implementations. And the RasterPart could simply accept any RasterElement or RasterElement.Provider and get the RasterElement and store that for RasterPart.image. This would make the underlying datastructure the same. And since everything generally delegates to the RasterPart rather than the RasterPart.image object directly, this would shift most of the internals to a consistent structure while bypassing the need to correct much else in the spaghetti code.

The BufferedImageAdapter class would still do the convert to grey natively. And the BlackWhiteRaster class would still do dithering natively. But, the results would just be tossed into a consistent RasterElement object and the rest of the classes would be ignored. Then just streamline the other elements. Since RasterPart.image would always be a RasterElement, this could be used for RasterBuilder directly or accessed for the .getRasterLine() data natively.

Then duplicating the graphics to greys and dithering algorithms in Visicut would just provide a way to skip using this spaghetti code. While Visicut is still tightly coupled with this where the dithering algorithm is directly referenced in the xml and so changing structures in the library will break the settings files in Visicut, it could all be left in place and just create a second much more reasonable pathway where using the older code simply jumps through those hoops and the RasterPart just gets the backing RasterElement or you could use the graphics functions with raw data access in Visicut and give the RasterPart the RasterElement directly.

Adding in a function for RasterizablePart of .asVectorCommands() that simply delegated to the RasterBuilder could then be used instead of the call to LaserCutter.convertRasterizableToVectorPart(). While none of this removes the spaghetti code it would make it all easy to remove. As there would be other better, modularized ways of doing all of these things. All drivers would have easy access to .asVectorCommands() which would just generate the commands on the fly with all the options they might want. If Visicut was extended to use a proper graphics library to do the several conversions to grey and several different dithers and show those results directly, then just send that to RasterPart.