scottrini / OctoPrint-PrusaLevelingGuide

42 stars 7 forks source link

MINI+Marlin support, rounding fixes, drop python2, clean readme #37

Closed acortelyou closed 6 months ago

acortelyou commented 3 years ago

I updated my repo so that the default branch includes all of the fixes I've added with a streamlined README.

The main difference from the previous PR is that this one drops Python2 support explicitly to prevent any upgrade issues and encourages users to update to OctoPi 0.18+.

I hope you will be able to merge this or I can leave my fork up for users who want accurate values and/or MINI+Marlin support.

scottrini commented 3 years ago

Sorry, let alone the changes to the README I wouldn't merge, I don't intend on having it require scipy. I will probably end up getting a version of your changes into the main repo, but without scipy as a dependency.

acortelyou commented 3 years ago

I can appreciate your position on scipy and the README, but the values you currently display are completely wrong for degrees.

You should remerge the rounding fixes ASAP.

scottrini commented 3 years ago

@acortelyou I still feel like we should work towards getting everything merged in, I just haven't had time the past few days, but I'll have some time today.

I've been a software engineer at one of the biggest for over ten years and the software was extremely legacy and supported hundreds of thousands (if not millions) of users. From my time there, I've become really sensitive to breaking anything legacy and that's been my hesitation here - I don't want something that users have been happily using for a while to suddenly cause them issues, I want them to be able to continue using it happily. I like how blocking out python2.x will just leave those users with the last known good python2.x version, so I'm warming up to this idea a bit more, it just bothers me slightly they won't get bug fixes unless I break this out into a py2 and py3 version.

I intend on looking into the method being used in scipy and trying to figure out an identical method we can drop in locally to remove the dependency. It's dirty to duplicate the function like that, but it's a pretty heavy dependency and with the issues everyone's seeing, I see it as a decent short term solution (hoping we'd be able to use the scipy dependency in the future). I don't have nearly as many problems leaving numpy as a dependency, as that wasn't my issue on any of the instances I tried and it's such a common dependency.

I know what you're referring to with the degree display - it's because I purposely changed this to round before calculating. It's off, but it's not horribly inaccurate. It was originally rounding after calculations, but I was getting complaints from users that the numbers didn't match the pcboy tool (the degrees). Looking into the source there, I saw they were rounding before calculating, so I duplicated it specifically to keep people from complaining that they didn't match - no one liked my answer that my numbers were more accurate since they'd been using the pcboy tool successfully for some time already. So that's exactly why it's at this state now, but I agree it should be more accurate. I've considered reverting that change from time to time since I added it, just to see if users would actually notice. But I completely agree, it should be rounding after, I just did this begrudgingly when I conceded to the community.

scottrini commented 3 years ago

@acortelyou So I actually now have a lot of your changes pulled in locally, but I haven't pushed anything yet. I'm going to get the rounding changes and a lot of the README changes pushed in first, then I have a few other small things to work on, but I'd like for us to continue working on getting the additional fw support pushed in. I'd like to pin up a dev branch to continue this.

After I finish up a few small things here, I'm going to pull down the scipy source and their unit test for RectBivariateSpline. I'm going to setup a little harness locally to run that test against the scipy version, then again against my own version and try to get an independent method working. If I can get that working, we can drop that in, remove scipy, and merge in all the changes.

Either way, I'll also plan to make a little contributor table in the README.md to include your name and contributions.

scottrini commented 3 years ago

@acortelyou I haven't forgotten about this. I still really want to get your changes in. I started messing with making my own replacement function, but I didn't get very far. I was hoping to spend a little more time on this today, but I wanted to open it up here to see if you had any recommendations.

Or more importantly - is there a way to completely remove the RectBivariateSpline function call for now? I'm trying to figure out how necessary it is. I was hoping maybe we could leave it out for now and that roll that in later after we get the main changes in and rolled out.

acortelyou commented 3 years ago

A bivariate spline is a clean and easy way to interpolate the values from the 7x7 and 4x4 mesh that the MK3 and MINI output and map it to the 3x3 grid required for silicone/nylock mod levelling. It uses all of the available data to create a model of the bed as a continuous surface and will give the most accurate result.

If you can find an implementation of a bivariate spline elsewhere you could use that instead, but I personally prefer to not re-invent the wheel.

Alternatives are more lossy: your original implementation simply took the closest available value to the screw and ignored the rest. Unfortunately that approach does not work for the MINI as a 4x4 grid does not have a single value corresponding to the cardinal directions. Other tools have worked around this by averaging the two closest values.

scottrini commented 3 years ago

@acortelyou So that's how it solves for the arbitrary mesh grid sizes, that makes a lot of sense.

I totally agree with reinventing the wheel, that's why I was just hoping as a temporary bandaid, I could get that method from scipy just working externally.

I plan on removing the python2 support very soon anyway, though. I merged in a new bed view (so themes and css won't screw with the alignment). So I created one with an html canvas. Once it's out of 'beta' and replaces the original bed view, I'm going to move forward with cutting off the python2 support anyway.

DarioX7 commented 2 years ago

Hello. Has something moved to support the Prusa MINI+ printer? I can see that the last post is over 1.5 years old.