Replacing big if else with regular expressions

[natdan]

@Benraay (and @nebbian): I've been thinking what to do with code in Slic3r.postProcess method. It is quite awful with all the assignments in conditional expression :eek:

I can propose two alternative - but none is ideal (Java is not the best with DSL - had it been Scala or some other language...) and cannot decide which would you like better.

First is simple DSL along the lines:

lineProcessor.add("^(M204\\s+S(\\d+))", (line, group1, group2) -> writeOutput(String.format("M201 X%d Y%d Z%d E2000\n", group2, group2, group2)));
lineProcessor.add("^(M190\\s)", () -> {}));
lineProcessor.add("^(G1\\s+E([\\-0-9\\.]+)\\s+F([0-9]+))", (line, g1, g2, g3) -> {
    double extrusion = Double.parseDouble(g2);
    int feedRate = Integer.parseInt(g3);
    ...
});

As you can see I failed to infer types for parameters - they all have to be String (due to type erasure, etc...)

Second option is to introduce PostProcessor class (AbstractPostProcessor or just public abstract PostProcessor) which is going to process file line by line but we can do type inference by methods. Instead of adding lambdas to lineProcessor DSL utility class we can use annotations and methods! For instance:

public class Slic3rPostProcessor extends PostProcessor {
...
    @Pattern("(X([\\-0-9\\.]+)\\s+Y([\\-0-9\\.]+))")
    public void commandDistance(Object ignore, double newX, double newY) {
        commandDistance = Math.sqrt(Math.pow(newX - currentX, 2) + Math.pow(newY - currentY, 2));
    }

    @Pattern("^(M204\\s+S(\\d+))")
    public void M201(Object ignore, String accel) {
        writeOutput(String.format("M201 X%d Y%d Z%d E2000\n", accel, accel, accel);
    }

    ....

}

I think there's a scope in handing groups 0 (the line) and 1 (which was never used in your regex statements),

Which one do you like better?

[Benraay]

Yes I managed to rewrite perl to Java like it was. I specially love the assignments in conditional expression :) There is probably a better solution like you are proposing.

The second one looks more robust

[natdan]

Oh - you did good job converting it in nice tight code... But to me assignments in conditional expression are really screaming code smell... LOL

I'll try to re-organise it in a way of introducing bit of both and create separate pull request for you to check if you like it. I am not insisting on such solution - just curious if we can make it more... I don't know, readable? Logical? If that solution ended up neither - than we'll just abandon it.

And - there'll be more code like that for other slicers so it should be quite generic...

[nebbian]

By the way, just to throw a fly in the ointment here, I've managed to rewrite the postprocessor so that most of those regular expressions aren't needed.

It's just fleshing out the G1_Disabled code that's there at the moment. It seems to work OK, I just need to do a bit more testing. I think it's far more robust than all those regular expressions that seem quite fragile. It should actually work with other slicers as well, provided we can get the layer number hints working.

[natdan]

Oh, don't worry - I've got time :) We'll do it at the appropriate point when it doesn't affect you doing real code...

[Benraay]

@natdan I was joking on the assignment in tests of corse it's a really bad usage.

@nebbian good news I was starting to do that yesterday until I saw natdan's code re-organisation

@nebbian I let you push your changes when you have time.

I'm working on a absolute to relative E convertion to handle the cura 2.5 code...

[natdan]

@Benraay you've got me there! LOL Please guys - do let me know if you feel I'm under your feet. I can refrain pushing some changes until you do yours... Or - if you believe there there's a good strategy how to go through first big bundle of code into @nebbian repo so we can easily work on our branches without tripping each other - do let me know! :)

[Benraay]

@natdan it's okay for me you make good improvements in the workflow no matter If I need I will make a branch to make my changes.

I will concentrate on the Absolute to relative E for now

[nebbian]

I just tried to push my new code to Natdan's fork in a new branch, but got the following message:

~/Dropbox/Apps/3D/Software/Robox/RoboxSlicerExtension_NatDan> git push --porcelain --progress --recurse-submodules=check origin refs/heads/Feature-Generic-Slicer:refs/heads/Feature-Generic-Slicer
'Feature-Generic-Slicer' rejected (non-fast-forward)
Delta compression using up to 2 threads.
Total 4 (delta 2), reused 0 (delta 0)
remote: Resolving deltas:   0% (0/2)           
remote: Resolving deltas:  50% (1/2)           
remote: Resolving deltas: 100% (2/2)           
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.        
error: failed to push some refs to 'git@github.com:natdan/RoboxSlicerExtension.git'
Not all refs have been pushed.

Any ideas?

[natdan]

No idea... You're trying to create new branch, right? Did you try it again? Maybe try just simple $ git push and follow instructions if it complains that you didn't set upstream branch... Or - use git gui! :)

[nebbian]

Yeah I was using Smartgit.

I used the same process, and made a new branch in my repo. Feel free to pull from upstream to see the new, simpler, code.

[natdan]

Will do. Since you've invited me to collaborate in your repo - shell we just move all the code there and make PRs from feature branches to master there?

[nebbian]

Yep, I think keeping it all in the one repo would be a good way to go. Using pull requests within the same repo seems like a great way to ensure that all the i's are dotted and t's crossed. I've just invited @Benraay as well.

[natdan]

@nebbian I've just pushed branch to your repo @Benraay I am sure that there's git incantation to do it through git commands but I just did:

[branch "Initial-Flow-Code"]
        remote = upstream
        merge = refs/heads/Initial-Flow-Code

change - remote above from 'origin' to 'upstream' (given that you have already defined upstream). If not just add following, too:

[remote "upstream"]
        url = git@github.com:nebbian/RoboxSlicerExtension.git
        fetch = +refs/heads/*:refs/remotes/upstream/*

[Benraay]

Ok but I have just written a few lines of code If I just clone the Initial-Flow-Code and paste my changes it can also be ok or not ?

[natdan]

Of course. Sometimes it is less hassle doing it the way you suggest! :)

[Benraay]

@natdan did you start the postProcessor class I want to rewrite latest perl version of @nebbian but it could be good to do it with the new structure you suggested. I'm not sure how exactly you want to organize it, Can you just make a sample code.

[natdan]

No, I didn't - weather was awful for coding (otherwise fantastic for barbecue). I'll do it tonight if you don't mind.

[Benraay]

Barbecue is great also ! I did it last week end ! No matter let me now when you have something.

[nebbian]

Just letting you guys know that I've just merged the new postprocessing code into the master branch. It seems to be working well, and for some reason it's producing better prints than the old, more complicated code, at least for one particular model I've been printing. This should be a much better candidate to start with when writing the Java port.

[Benraay]

Yes this code is more generic and maybe more efficient ! I will try to port this tonight and move the stuff in master class. and also make some tests with cura 2.5 if I have some time.

We are going to have a nice plugin !

[natdan]

OK. I've done something. It compiles but I didn't get to test it out yet. Do have a look at Slic3rPostProcessor (and impl AbstractPostProcessor). Also, there's new class LineProcessor. Do let me know if it makes sense! :)

[Benraay]

@natdan and @nebbian I have ported the perl code from @nebbian to Java and moves all the stuff to Slicer Class because all this code is generic now.

@natdan I have read all of your implementation on the Pattern and line postProcessor. I'm sorry but my feeling is that we loose the readability of the processor in that way it's probably much more "Java way of coding" but more difficult to change it.

It took me some time to understand how you managed this and there is a lot of code around to make it work with the patterns.

I don't know what you think about that but with the new perl implementation there are less lines of code, I have moved most of declarations outside tests.

We need to keep it simple and easy to maintain !

[natdan]

@Benraay It is up to you. If you think it is not clean or easy to understand, i.e. intuitive, (not the mechanism behind it but the class that does the transformation) then leave it away.

Oh, BTW, ever so slightly related - all those need to be refactored to different packages, but we don't have to do it just yet. :)

[Benraay]

Yes we probably need to make some refactoring at the end.

I'm sorry for the work you made on the patterns but I prefer to keep it a bit more simple to understand.

I just pushed a new version and added a little intall bash script that copies the jar and the bash script. for now the bash script is calling the jar with all the options, I found a better way to cleanup the parameters, I had to change some things in the java code also, like keeping .gcode at the end of the file if not Slic3r adds .gcode extension at the end.

I does not print for now because of some bug on post processing but all the process is working.

[natdan]

Oh, don't worry. I'm sure I'll be able to make you like that other solution - you just need more time! ;)

However, main thing is that I don't want to be in the way until we have nice, stable working code. All of that pattern thing is extra stuff and not quite yet needed.

[Benraay]

lol ok I let you the time to convince me later...

[nebbian]

I believe this issue is resolved for the moment. I'm very happy with the new java code :)

[Benraay]

for the moment it's working nice !