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!
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher Offset

Post by abdullah »

paddle wrote: Mon Jan 30, 2023 10:01 am 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.
You implied it, because precisely sketcher offset is part of a extremely big PR that never ended growing. Reality is seldom simple. As said, I do not regret the time invested. I would do it again for you or for any other showing interest in the sketcher.
paddle wrote: Mon Jan 30, 2023 10:01 am 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.
There are different types of PRs:
- There are PRs which touch a very well defined area. As far as the class interface is ok and code is not specially ugly, I can go along with it if it works.
- There are PRs that spread over quite some extent. Here the structure of the code needs to be sound. Not perfect, just sound. I needs not to force maintainers to have to rewrite it.
- There are PRs that touch a complete hierarchy of classes over different WBs. Here architecture is a must. There are no buts. You may call me names.

You have a taste for the later two types. Complexity needs to be managed on those types of PRs.

Now, you come to the grid PR. First you do "the job". I can agree it was shiny on the outside. Then, you took out of context an indication from me that if the PR was going to touch Coin, there should be something similar to a coinmanager. Later, I reviewed part of the code and found that a lot of Grid related properties had been added to ViewProviderSketch. I stopped reviewing and asked for an opinion from Werner. I was never convinced that these properties should be there. At the end, Werner's opinion is there for anybody to read. To some extend, it confirms that is not ok (a breach of the single responsibility principle, SRP).

So, what you call "a weird inheritance of VPsketch" is actually a breach of the SRP. The SRP is a real thing, you may want to Google it... or wait, probably you can go back to our many discussions. One class should have one responsibility. Sketcher's coinManager responsibility is about representing sketcher features. The grid is not a sketcher feature, it is just a grid. It is even optional.

You may have a point that I could have done things differently (better communication). However, that would have required even more time when I was close to a burnout from may other PRs. You can blame me on that. I will take it.

The latest sentence embodies one of your biggest shortcomings: you just do not care about Architecture. It is not that you cannot learn about it. You are more than smart enough. You just do not want to learn about it because for you it is complexity, rather than a way to handle complexity and preventing code to grow out of control (which is what it actually is). I failed to help you understand that. I tried, but did not succeed.

You produce shiny features, but you minimise the investment beyond the appearance. Maintainers need you to invest more time into producing good quality code, so that features can be merged with a small number of iterations or none.
paddle wrote: Mon Jan 30, 2023 10:01 am 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.
Again, this is the wrong approach to it. I am not merging code just because code is there waiting to be merged. Any code cannot be merged. If we were 10 people full time, somebody would have fixed the PR already and merged it. Reality is different. The consequence is that certain PRs need to wait.
adrianinsaval wrote: Mon Jan 30, 2023 12:43 pm 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.
If there is any blame to be attributed, I should be the recipient. I do not accept that OpenBrain is to any kind of blame. If anything, I should have supported OpenBrain better, as he got a very difficult assignment. Not all PRs are the same. Beyond that, OpenBrain has helped Paddle a lot over time. He has reviewed or helped me review other PRs of Paddle. OpenBrain has coded snippets for Paddle to help him fix issues. I have no doubt his intentions were good.

The fact that Paddle moved code to a coinmanager is not per se wrong. The problem is that that coinmanager should be in a separate class dealing with the Grid (either a ViewProvider or a ViewProviderExtension, the latter is Werner's pick). Properties should not be moved to ViewProviderSketch. When ViewProviderSketch inherings from another ViewProvider or a ViewProviderExtension, those properties "are" part of ViewProviderSketch, but are not defined in ViewProviderSketch. This allows to achieve two goals: (a) all the grid functionality is in one separate class/extension, so it can be leveraged by any other ViewProvider in the future, (b) it prevents unnecessary growing of ViewProviderSketch, which means more code to maintain and poor separation of responsibilities (because without having to conform to an interface between objects, developers end up inter-tangling different responsibilities making it very tedious to untangle them later on). I spend around two months untangling ViewProviderSketch one year ago (and I did not untangle everything I wanted to).

That said, the decision to move code to a coinmanager was not a bad decision. It is just unfinished. It is just one more thing that needed to be tackled in that PR (and one can read the whole story, as it is available in GH, there were many things tackled).
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: Sketcher Offset

Post by paddle »

abdullah wrote: Mon Jan 30, 2023 5:33 pm The latest sentence embodies one of your biggest shortcomings: you just do not care about Architecture. It is not that you cannot learn about it. You are more than smart enough. You just do not want to learn about it because for you it is complexity, rather than a way to handle complexity and preventing code to grow out of control (which is what it actually is). I failed to help you understand that. I tried, but did not succeed.
I didn't disagree that architecture isn't important. I just kept trying to argue that there should be a balance between accepted code quality and getting things done. Grid PR is the perfect example of overdoing it. Putting it in part wb would make sense if it was indeed a shared object. But it's not. Nothing else use sketcher grid.

You guys say my grid PR architecture is not acceptable, when you look at current situation how can it be said ? What on earth is 'Grid Style' property? What sense does it make? "Max number of lines = 10 000" property ? What kind of crappy hack is that? How did that even got merged in the first place ?

Lastly you speak as if I refuse to comply to the obvious code architecture, as if freecad code architecture was a well documented, clear to understand concept. It's not. It's intricated, unclear, unintuitive in many places. And when I ask for directives, I often just get no reply for days, weeks, never. So what am I to do? I just make as I can.

I'm sorry you seem to be upset about my message. It wasn't my intention. I just stated how things were to someone who asked when it would merge. I mean if he needs offset in his workflow today, I can't tell him it's going to merge soon.
abdullah
Veteran
Posts: 4935
Joined: Sun May 04, 2014 3:16 pm
Contact:

Re: Sketcher Offset

Post by abdullah »

paddle wrote: Mon Jan 30, 2023 10:01 pm I didn't disagree that architecture isn't important. I just kept trying to argue that there should be a balance between accepted code quality and getting things done. Grid PR is the perfect example of overdoing it. Putting it in part wb would make sense if it was indeed a shared object. But it's not. Nothing else use sketcher grid.
Often you criticise architecture and when others talk about it. You seem to follow an own understanding of what is reasonable or not as architecture. Because of this very narrow understanding, you argue to seek a balance. The rest is obviously overdoing it. Here I can only tell you that several others do not agree with your understanding. It can be that we are all wrong. Unavoidably, I perceive that what is important to me, it is not important to you.

I will try once more. Grid PR is a very good example of you not understanding architecture and I do not need to show code to argue this point. You say that because the Grid code is not being reused at the moment, it is ok to copy paste the code that was before in Part namespace into the Sketcher namespace, including all properties defining the grid, and not only that, to copy it directly into the ViewProvider (or the coinManager delegate class of this ViewProvider).

You see this being natural, as "having to look for code in two places" is complexity for you. What I am trying to tell you is that separating different functions into different classes, so that one class (or actor) takes one responsibility and just one, is called Single Responsibility Principle (is one of the SOLID principles of software architecture). This splitting makes each class less complex, and clearly defines the boundaries between the two. In addition, it enables code reuse (which is an extra). Moreover, provides extra encapsulation for details (see private member and functions and inheritance). It helps to deal with complexity and enables ordered growth of code.

I am not going to go one by one to your claims, because the solution when one's shortcomings are exposed, is not to try to hit back on the shortcomings of others, but rather to fix one's shortcomings. However, I can tell you that an encapsulated hack is better than bad architecture. An encapsulated hack can be easily corrected in the future. Bad architecture cannot. As an example, you remember we had a problem with one of your PRs with what appeared to be an issue of QT. There was a hackish work-around. We encapsulated it and it got merged. No, we do not seek perfect code. We seek average maintainable code.
User avatar
adrianinsaval
Veteran
Posts: 5541
Joined: Thu Apr 05, 2018 5:15 pm

Re: Sketcher Offset

Post by adrianinsaval »

abdullah wrote: Tue Jan 31, 2023 3:31 pm You say that because the Grid code is not being reused at the moment, it is ok to copy paste the code that was before in Part namespace into the Sketcher namespace, including all properties defining the grid, and not only that, to copy it directly into the ViewProvider (or the coinManager delegate class of this ViewProvider).
Note that this was done on another maintainer's instructions, initially paddle implemented a good portion of his changes in Part initially, while I agree paddle should care more about architecture it is unfair to put all the blame for the current architecture of that PR on him when he was following instructions from a maintainer
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Sketcher Offset

Post by openBrain »

Just saw this thread. :) Will drop a few words. Maybe will be a long post. :) In any case just expect me posting the Latin way. Not everyone can be as wise as abdullah. :lol:
paddle wrote: Thu Jan 26, 2023 5:05 pm For the record, I personally disagree with this way of doing things. It is so slow it's killing development.
Saying so both proves inexperience in development and surely won't encourage anyone merging PR.
As said GeneFC, while it may prevent some (functionally) interesting PR to be merged, it is efficiently protecting the codebase to become unmaintainable, which for sure will kill the development.
Also not very lucid (humble ?) to consider that 2 or 3 PR from the same contributor can be considered as "the development" of FreeCAD. As I see it, in the last months most PRs have been merged without any issue.
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.
Is it good situation ? No. But unfortunately it is acceptable. Merging anything regardless of its quality won't help sustain the project (would be worse actually). New devs would get : 1) garbage codebase that would be impossible to understand, so to hack 2) PR unmerged because all maintainer workforce would be busy fixing issues introduced by other PRs
abdullah wrote: Fri Jan 27, 2023 8:47 pm 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...).
Feel free to call me "madame" if it makes you feel better. :P
paddle wrote: Mon Jan 30, 2023 10:01 am 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.
I'm a bit disappointed of that. Yes there has been miscommunication and probably bad advice/decisions during this PR. It was admitted. But after 1+ month of fresh air breathing, you still come with this bitterness not very likely to help merging the PR.
I told that if architecture was the only way preventing merge, I will propose to do the refactoring. Finally @abdullah took it and I'll support him in doing so (if needed). But why are you not able to come back also in a constructive approach, saying "Guys, OK, I'm ready to put another effort to help. How can I ?"
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.
Hmmm. As we are not 10 people full-time, we have to rely on the contributor to help achieve an acceptable PR. No more, no less.
adrianinsaval wrote: Mon Jan 30, 2023 12:43 pm 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.
I'm not sure that wouldn't happen again. The grid PR is a good example of what a PR should not be. This should be split into 3 or 4 PRs (eventually depending on each other, git handles this very well). Also there has been lot of time consumed in this PR fixing issues, most of them being easy to see with some functional testing. If architecture has been the only issue of the PR, it would have been very different I'm sure.
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.
Most of the restructure that was done is still valid. Just some more is needed (basically). Yes it was not told at the beginning and can look discouraging. Again, if PR has been well tailored and architecture was the only problem, the review on this would probably has been more complete. ;)
paddle wrote: Mon Jan 30, 2023 2:24 pm 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.
The main problem here is probably that you didn't show any willing to continue working on this code if it gets merged. Oppositely you clearly expressed that absolutely don't want to spend time on improving its architecture that you found useless effort because "hey, it works" -- in the nominal case --.
Do you really think it may encourage someone merge with remaining problems ?
paddle wrote: Mon Jan 30, 2023 10:01 pm I didn't disagree that architecture isn't important.
This double (triple ?) negation is a funny slip of the tongue. :D Maybe it says it all. :lol:
You guys say my grid PR architecture is not acceptable, when you look at current situation how can it be said ? What on earth is 'Grid Style' property? What sense does it make? "Max number of lines = 10 000" property ? What kind of crappy hack is that? How did that even got merged in the first place ?
Image
Maintainer : - Hey guy, you just wreck my car !
Paddle : - Hmpf. There was already a scratch on it. So who cares ? Nevermind.
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Sketcher Offset

Post by openBrain »

Now if some want to have a bigger picture of what kind of problems we are trying to avoid, you can have a look at https://github.com/FreeCAD/FreeCAD/pull/7566
This is a good example.

In this PR (that already was a big effort in reviewing), some extra changes (globally not linked to initial changes proposed in the PR) were added after the initial review.
Then the PR was merged using some fallacious arguing that PR has been reviewed.
Obviously, latter introduced changes that looked innocent brought a big bunch of issues.
The simplest solution would have been to revert, but guess what happened ???
One of this nasty maintainers (even not the one that merged) took time to quickly fix the introduced regressions : https://github.com/FreeCAD/FreeCAD/pull/7886
Notice that some minor issues/regressions were not fixed but identified in the fixing PR.
Now guess what the contributor is doing ? Doing some effort to fix the last issues he introduced Complaining about his PR not being merged.

Don't get it wrong. This is a bit caricatural and I have no personal anger to anyone. :) Just to show that other's point of view may need more information to be understood. ;)

And in case one can think I'm not able to understand contributor frustration : https://github.com/FreeCAD/FreeCAD/pull/7255
I have this PR opened for 6+ months. This is a bugfix PR (not only an improvement one). I already went through 2 huge merge conflicts resolution with it.
And still why didn't I dare to merge it myself while I have the rights to do so ? Because I want someone to acknowledge the code quality of it.
Hologram
Posts: 201
Joined: Thu Nov 03, 2022 3:05 pm

Re: Sketcher Offset

Post by Hologram »

May I advise you guys to let the past rest, note the things that went wrong or could have been done better and move forward with that?
It doesn't look good to an outsider seeing all this arguing. I understand all your points and positions, but it's starting to get somewhat personal, that won't help.
User avatar
paddle
Veteran
Posts: 1391
Joined: Mon Feb 03, 2020 4:47 pm

Re: Sketcher Offset

Post by paddle »

openBrain wrote: Wed Feb 01, 2023 3:04 pm Notice that some minor issues/regressions were not fixed but identified in the fixing PR.
Now guess what the contributor is doing ? Doing some effort to fix the last issues he introduced Complaining about his PR not being merged.
Sorry you misunderstood, I'm not complaining that the PR is not merged, I don't care anymore. Someone pinged me to ask when offset's going to merge, I just answered him.
After that I just replied. And in my answer I indeed complained that I have been asked to change the architecture twice for this PR before being asked a third time because afterall it's still not good. Am I bitter? Of course. Not merging that PR is pissing on my work and laughing at the fact that you made me change twice the architecture for nothing. Your picture of a wrecked car is also unfair. It's more like this : a guy with a wrecked crappy car say no to a free car because it has an invisible scratch in it.

Besides I gave up fc dev before constraint widget PR merged. If I had not I would of course have fixed issues. Besides I didn't trick anyone into merging it as you make it sound... You are drawing an unfair picture of me.

Anyway I got a reminder why I left in the first place, cheers. :arrow:
openBrain
Veteran
Posts: 9034
Joined: Fri Nov 09, 2018 5:38 pm
Contact:

Re: Sketcher Offset

Post by openBrain »

paddle wrote: Wed Feb 01, 2023 5:34 pm Sorry you misunderstood, I'm not complaining that the PR is not merged, I don't care anymore. Someone pinged me to ask when offset's going to merge, I just answered him.
I guess you're basically right. ;)
After that I just replied. And in my answer I indeed complained that I have been asked to change the architecture twice for this PR before being asked a third time because afterall it's still not good.
I remember only one. Other was some adjustments IIRC.
Notice that, as abdullah and I explained, the pending request isn't to "change to something else" but to "push further". This won't make the previous work useless.
I know this request came late and can make you feel frustrated. But as I said, it would probably have been more fluent if the architectural issue had been the only flaw in the PR. I can't believe just fixing architecture can generate more than 380 comments. ;)
Am I bitter? Of course. Not merging that PR is pissing on my work and laughing at the fact that you made me change twice the architecture for nothing.
It's not. It would be if it was kept unmerged for no reason. Here you know the reason, and what is expected to get it merged.
Your picture of a wrecked car is also unfair. It's more like this : a guy with a wrecked crappy car say no to a free car because it has an invisible scratch in it.
May be. :) However, believe me that I would strongly refuse to become the owner of a free shiny car that I know has architectural issues and will cost me 5 times the price of a new car in the coming months. ;)
Besides I didn't trick anyone into merging it as you make it sound... You are drawing an unfair picture of me.
:?
Anyway I got a reminder why I left in the first place, cheers. :arrow:
That's a bit of a pity because I'm sure you have time and skills to be a valuable FreeCAD contributor (why I also invest a significant amount of time guiding). This said, if you're not ready to move some of your lines, we'll always fall on the same hurdles, which obviously isn't sustainable.

BTW : if anyone think there is a problem with contribution handling, feel free to open a documented issue in GH.
User avatar
NewJoker
Veteran
Posts: 3014
Joined: Sun Oct 11, 2020 7:49 pm

Re: Sketcher Offset

Post by NewJoker »

paddle wrote: Wed Feb 01, 2023 5:34 pm Besides I gave up fc dev before constraint widget PR merged. If I had not I would of course have fixed issues.
Too bad, you are the only chance for the Sketcher to become as powerful as in commercial software. Without your involvement, missing functionalities such as offset, circular pattern, chamfer and many others will likely never be added and a lot of great work will be wasted. Perhaps instead of giving up completely and leaving your code to rot, you could for example create your own branch like Realthunder did. Or even better - ask him to merge your PRs into his version of FreeCAD. He is much more open-minded than the mergers of the main branch and should appreciate your efforts. You could also consider asking for donations like jnxd did to develop some tools for B-spline handling in the Sketcher. His fundraising was really successful. And there's still a chance that some people will join to help you if we spread the news about your brilliant ideas for improvements.
Post Reply