Sketcher Offset

Info about new community or project announcements, implemented features, classes, modules or APIs. Might get technical!
PLEASE DO NOT POST HELP REQUESTS OR OTHER DISCUSSIONS HERE!
Forum rules
Be nice to others! Respect the FreeCAD code of conduct!
Post Reply
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: Sketcher Offset

Post by paddle »

Hologram wrote: Wed Jan 25, 2023 11:20 pm @paddle Just checked the latest 0.21 build, do you have any timeline for the merger of the auto-constraint and mouse group dragging features? Was hoping these would already be in master :)
As I mentioned before, this is sadly out of my hands. You need to ask Abdullah the sketcher maintainer/merger.

At the rate things are merged don't hold your breath. They are very picky on code, even when something works perfectly they won't merge because the code architecture isn't exactly what they personally want.

For the record, I personally disagree with this way of doing things. It is so slow it's killing development.
GeneFC
Veteran
Posts: 5373
Joined: Sat Mar 19, 2016 3:36 pm
Location: Punta Gorda, FL

Re: Sketcher Offset

Post by GeneFC »

paddle wrote: Thu Jan 26, 2023 5:05 pm For the record, I personally disagree with this way of doing things.
It might be slow, but it helps prevent (or slow) the code from becoming an unmaintainable garbage dump.

Gene
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Sketcher Offset

Post by adrianinsaval »

it also keeps new devs away making it harder to sustain the project and maintain the code so benefits are somewhat questionable, we had discussed and agreed on a contribution process, but sadly none of it was followed for these cases, neither from the contributor nor from the maintainers. Now many good features are in the limbo with a developer that has lost motivation due to failure of the process.
Hologram
Posts: 201
Joined: Thu Nov 03, 2022 3:05 pm

Re: Sketcher Offset

Post by Hologram »

Ideally, both sides will be kept satisfied. That'll mean taking the middle ground; adopting patches relatively easily, yet with maintainable code. Easier said than done, I know.
But from a user point of view @paddle created things that even Fusion 360 struggles with. That's really nice and a way to stand out from the crowd. These are really the selling, exiting features for end users.

Hope it's okay to ping @abdullah to discuss the merging process for the features in this topic :?
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher Offset

Post by abdullah »

I can agree I am not the faster merger here. That one is on me. I have limited time.

No, I am not "picky" with the code. You may ask other contributors. When things are within the contribution guidelines, merges come reasonably fast (at least when I am not sick or worse, like last summer). Even highly transcendental changes of mathematically demanding code (such as Ajinkya's B-Spline solver support PRs) have been merged in about two months (including improvement cycles). I try to care about code maintainability and I put an emphasis in software architecture, because I know the consequences of not doing it. I put a lot of thought into backward and forward compatibility, because I think the Sketcher is one of the most essential parts of FreeCAD, if not the most, and problem in the sketcher would have a very strong impact in FreeCAD as a whole.

Paddle's PRs have had tons of problems that, if merged as is, would have made future maintainability a nightmare. No, it is not a matter of "preference". That is very unfair, specially taking into account the amount of hours I have invested in reviewing code, exchanging PMs explaining problems and providing solutions. I have explained several times the reasons why software architecture is important. Other argument I needed to hear is that other parts of the Sketcher are not that shiny, so I was being unfair to him because, that was his position, I was demanding more of him than of others. Even if it were true that parts of the Sketcher should be refactored, why on earth should that be an argument to merge code with insufficient quality? The solution is to fix the issues so that the code is acceptable, not to try to push it as is.

I can agree Paddle has been consistent in disregarding software architecture. As he has claimed several times, he does not deem it important. After all, he appears to believe, it is the task of the maintainers to invest substantial effort in fixing his PRs or deal with the consequences of merging them. Last year most of my FC investment has gone into him. It is not a complaint. I considered it an investment. I hoped I would be able to convince him to do things differently. I have not managed to. Still it was my choice and I feel no regrets. I would do it again.

However, my decision to invest all my FC time on him did have consequences. Ajinkya was the most negatively impacted by my decision. Some of his simpler B-Spline PRs were on the pipeline for months, as I did not have time left for him first, then I got sick and when I recovered, there were more Paddle PRs waiting. And that is a real pity indeed, because Ajinkya's PR's were closer to mergable. I was really unfair to him (and I am sorry). Yet Ajinkya did not complain (even though he had every reason to). Conversely, Paddle complained once and again. I was closed to a burnout end of the last year. I think I was not the only maintainer.

So Paddle is abhorrent? No, not at all. Paddle is brilliant in many ways. He has an eye about how to make meaningful changes. He is fast coding and can find his way through code complexity. He is very resourceful in hacking away code. Unfortunately, part of the code is such a hack that needs rewriting. I wanted to arrive to the point were he would realise this and fix his PRs before submitting them. To me he has a bad balance of productivity and quality, but with a huge potential of improvement.

Now, this may counter some positions, but brings us no closer to having code and features merged.
adrianinsaval wrote: Fri Jan 27, 2023 2:09 pm it also keeps new devs away making it harder to sustain the project and maintain the code so benefits are somewhat questionable, we had discussed and agreed on a contribution process, but sadly none of it was followed for these cases, neither from the contributor nor from the maintainers. Now many good features are in the limbo with a developer that has lost motivation due to failure of the process.
There is one thing we do need to understand: It is harder to sustain the project if new devs' PRs are unmergeable or require substantial changes, than with lesser devs. This is not a call for lesser devs. I fo prefer more devs. However, it is a call for balance. We gain nothing if our maintainers end up with a burnout.

I understand that the contribution process has a potential to be used to shame the maintainer and blame him or her (why don't we have female maintainers here? Must be the contribution process...). Almost everything can be argued to be "preferences". Here I would like to warn against weaponising the contribution process. Yes, maintainers need to be held accountable. But code quality is not a "preference". No, a maintainer is not required to produce a counter PR if quality is not ok. He has a substantial amount of discretion (not arbitrariness), which is necessary to do his work.

However, to be fair to everybody, including Paddle, we discussed a contribution process way after most of Paddle's PRs. His latest PRs are way better than the earlier ones. The ones that could be fixed without substantial time investments were merged. The ones that have issues need those issues addressed. I said I would do that after I PR the NotificationArea (I have the time I have, and no more). If all the PRs were of acceptable quality, much would be already merged, it not all.

Anyway, it is not just the process that defines the outcome. The quality of the code is the most important factor. A huge PR can be merged if the quality is such that no objection can be raised. However, huge PRs having lots of problems have low chances and cost a lot of iterations, time and effort of the maintainers. Similarly, it has happened that, as a result of asking a change, we got several other unasked changes which forced to start the review anew. That is also very demotivating for maintainers. The process is a means to try to avoid to reach those cases. I do not think "the process failed". The process was not conceived to treat the cases that failed. The persons failed in not adhering to the process, or failing to produce code of sufficient quality. The process does not write the code.

Am I happy that Paddle lost his motivation? Not at all. Is there anything I could have done beyond what I did? I do not think so. Are there good ideas in the limbo? Yes, there are. Are they lost? No, they aren't. Will they be merged tomorrow? Not really in their present form. Then when? When they morph into an acceptable form.
Hologram wrote: Fri Jan 27, 2023 2:38 pm Ideally, both sides will be kept satisfied. That'll mean taking the middle ground; adopting patches relatively easily, yet with maintainable code. Easier said than done, I know.
But from a user point of view @paddle created things that even Fusion 360 struggles with. That's really nice and a way to stand out from the crowd. These are really the selling, exiting features for end users.
Yes. Paddle is a very intelligent individual and has very good ideas. He has also improved his coding skills over the last year and his knowledge of FreeCAD. He has a lot of potential to produce great features. He tries to push his boundaries, which is great for his learning. However, often he ends up with more code complexity than he can manage, which leads to code problems. He tends to try to overachieve in PRs, which results in an accumulation of code problems. He can indeed produce very shiny features. He would need to put more emphasis into code quality and code architecture. Here, it is a major hurdle that he does not believe in that need.

As I see it, we come from an almost burnout. There is a need to revisit Paddle PRs. Time will tell how everybody finds his way in the project. That is what is anyway what I can offer ATM.
Hologram
Posts: 201
Joined: Thu Nov 03, 2022 3:05 pm

Re: Sketcher Offset

Post by Hologram »

Thank you for your honest words Abdullah. I don't do it justice with a short message, but a sincere thanks to everyone involved.
Also, take good care of your health, please.

Don't want to meddle in this too much — especially as a non-developper — but sorry, I can't help that I'm pretty darn exited for your features Paddle. It's really rare to see people who create amazing QoL polish and workflow improvements =b

From reading between the lines, the main take-away for quicker merges might be for @paddle to look into his earlier merge requests again and see if, with the knowledge gained, he could better prepare them for a merge. It also reads as though, if the features could be split into more bite-sized chunks, it would be easier to do code reviews on them (i.e. be less time consuming).
I'll take my hands off this now, but will try to finish a new dark-mode theme in return ;)
User avatar
onekk
Veteran
Posts: 6144
Joined: Sat Jan 17, 2015 7:48 am
Contact:

Re: Sketcher Offset

Post by onekk »

Some features are sort of "deep changes" so they could not be split in pieces, if not carefully taylored to be modular.

Often this process need to rethink code or refactor it.

This coulb be nit very clear to "non coders" and even for some "new coders".

It is not a case that is called "Architecture" and that good code could not be written by AI.

Maintainable code should be a priority, as a developer could became unavailable for countless reasons as recent pandemic has shown in many cases.

What it lacks in @abdullah post is that sadly it is difficult to write code as there is lack of documentation about the way FreeCAD is coded.

This is a thing that some time has to be addressed, and probably would be next big effort as developers are not eternal so "new developers" have to be taught or at least helped to grow.

I've not C++ ability so sadly I can't help too much in these tasks.

Regards

Carlo D.
GitHub page: https://github.com/onekk/freecad-doc.
- In deep articles on FreeCAD.
- Learning how to model with scripting.
- Various other stuffs.

Blog: https://okkmkblog.wordpress.com/
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: Sketcher Offset

Post by paddle »

abdullah wrote: Fri Jan 27, 2023 8:47 pmNo, I am not "picky" with the code. You may ask other contributors.
I didn't say all my PR were good to go. I know a lot of the early stuff was not acceptable. And I thank you a lot for helping me and explaining things to me.

But yes, I still say that you guys are too picky on code architecture, because even when the PR is acceptable, if it doesn't match what you deam the exact optimal solution then it doesn't merge.

Look at the grid PR. I first do the job. I'm told it's not good because it's not in a coinmanager and bloats the VPsketch. I do it and what? I'm told again that this is not good.
Besides, having the grid drawn by a coinmanager as every other things in sketcher does makes sense. More so than having a weird inheritance of VPsketch. So you can't say that the PR in it's current state has an unacceptable architecture. It's just not matching your opinion of optimal.

If you were 10 people full time with the bandwidth to go the extra-lenght to spend 20 hours rewriting the grid PR to this optimal version, why not. But as it stands maintainers don't have the bandwidth to do so. So yes I don't think that being this picky is worth it.
Last edited by paddle on Mon Jan 30, 2023 2:28 pm, edited 1 time in total.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Sketcher Offset

Post by adrianinsaval »

I will support paddle here on the case of the grid PR (which I followed closely), but I don't think this is a generalized problem, this was being handled by a very new maintainer (openBrain) not abdullah, so for starters I think it's unfair to put all this on abdullah. I also don't think we need to crucify openBrain or the entire maintainers team over this, he made a mistake (which is to be expected from anyone starting on something new) and I hope he learns from it but I believe his intentions were good.
But we must at least recognize that this was very unfair to paddle as he was made to reestructure his PR unnecessarily and into an architecture that was then deemed incorrect by other maintainers. I think an apology and "let's all try to do better next time" is what is needed.
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: Sketcher Offset

Post by paddle »

Don't get me wrong, I'm not putting blame on anyone, I'm not angry or such. Everyone here is volonteer and does what he thinks best. I know OpenBrain and Abdullah spent a lot of time reviewing the stuff I wrote and fixing it and I appreciate it a lot.

I just stated my opinion on the merging policy because I think it's not the most productive way of doing things.

In my opinion this is the 'Ariane 6 / SLS' way of doing things. Making things 100% perfect before going forward. You build a really perfect rocket, but it takes 30 years. And you risk that your rocket is obsolete before it's even finished. I prefer the 'Starship' way of doing fix: getting things done asap, and fixing as we go.
Post Reply