search

Eneroth3 / eneroth-solid-tools

Solid operations for SketchUp
MIT License
26 stars 3 forks source link

improved Eneroth solids tools for review #1

Open LuluWalls opened 6 years ago

LuluWalls commented 6 years ago

@Eneroth3

I've upgraded many of the methods in solids.rb. Please review the code and let me know what you would like to change/update/throw away.

https://github.com/LuluWalls/Improved-Eneroth-Solid-Tools

I was hesitant to branch your repo with the large number of changes. Best Lulu

Eneroth3 commented 6 years ago

Hi Lulu

I'm very glad you want to contribute to the solid tools! I had almost forgot I had open sourced them as I haven't gotten much response before.

It is however quite hard to review the code when it is not forked from the original repo. Usually in a Git workflow a fork is made, a new branch is created and each change is added as an individual commit. This makes it very easy to see exactly what lines of code changed in each commit, and why (why is explained by the commit message).

There seems to be some potential here though, even if there are a few UX concerns. Also I think I need to clean up, refactor and better document the original code before anything new can be built onto it. It is a quite old code base and my skills have improved quite a bit since it was started. In the long run good looking and easily maintainable code is one of the most important aspects of a project.

I'll make some changes to the original code, and start an issue discussing how "multi-operations" (or what we should call them) could be designed/implemented, and tag you in it.

Thanks for showing interest!

Eneroth3 commented 6 years ago

I'm half way through cleaning/documenting/refactoring the current code base. Had I known you wanted to make an addition I would have done this before! I'm however not sure I can get this done today.

LuluWalls commented 6 years ago

There is absolutely no urgency on my part. I have a pile of phylogenetic statistics work waiting for attention, so I won't revisit this project for a few months. One note, the 'Dave Method', scaling objects up before intersecting, is baked in the new solids.rb methods. This might be a breaking change if you have done the scaling externally in another extension.

On Thursday, March 29, 2018, 10:43:11 AM EDT, Eneroth3 <notifications@github.com> wrote:

I'm half way through cleaning/documenting/refactoring the current code base. Had I known you wanted to make an addition I would have done this before! I'm however not sure I can get this done today.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

Eneroth3 commented 6 years ago

I have finished the much needed cleaning of the existing code base and looked into your version.

The code itself in "Improved-Eneroth-Solid-Tools" can't be used for a number of reasons. First of all it was based on a quite badly written code base in much need of refactoring and overall cleaning. Secondly individual changes cannot be reviewed on their own as version control wasn't used when creating them.

Some of the changes don't align with the goals of this project. Introducing a new tool that handles special cases of subtract, rather than extending all tools, make the extension harder to use with seemingly duplicated functionality and newly introduced inconsistency. Applying materials to individual faces are outside the scope of this project (the native tools doing this was even one of the main reasons to create this alternative in the first place). Instead it is up to the user to paint the faces inside of the cutting object. The spinning cursor thingy as feedback while carrying out operations doesn't look native to either the OS or SU and therefore adds unneeded noise to the user experience.

Lastly the coding style would make it very hard to maintain the code and build upon it in the future. Problems here include very long lines, commented out comments and odd and hard to read expressions such as & 0x01. It needs to be stated though that some of these issues could be caused by the original code base not having the highest quality.

However there are several ideas from "Improved-Eneroth-Solid-Tools" that could be used in Eneroth Solid Tools, and good models to test the implementations on.

The next time you participate in an open source project it might be a good idea to start an issue discussing the suggestions before implementing them, to see if they are in line with the project goals.

Version control is also highly useful. Make a fork, start a new branch (one per feature or bugfix) and make many and small commits, and it it will then be possible to see exactly what has changed when and why.

I'd also recomend SketchUp/rubocop-sketchup to help increase the general quality of the code.