nathanjshaffer
Newbie
United States
Joined: Dec 20, 2020
Post Count: 6
Status:
Offline
|
|
Re: Patch submission
|
To address your comments:
- Maybe you didn't notice, but the API of the model layer depends only on very basic Java classes, and not on classes of java.awt.geom package I did nit notice that as a design coice. I saw that you have used instances of Line2D from java.awt.geom so did not see a reason inheriting would be an issue. I respect your design choice but it does make things somewhat limiting. I Suppose all of the java.awt.geom Line2D/Point2D classes could be reimplemented without inheritance to satisfy that concern
- Naming of Vector2D is questionnable since it's a point not a vector. The class does treat itself as a vector, since adding 2 Vector2D instances returns a new Vector2D using vector math. A 2d vector is just an x,y magnitude and can function as a point equally. Programatically a vector is a point with extra math functions. Of course I could just call it Point2D.
- You seem to have not applied standard conventions to your code It has been a long time(18yr...) since I have programmed any Java. I am not as familiar with conventions. I knew in the back of my head I needed to do some cleanup and debugging. Please treat this as a proof of concept, not as a finished patch. It took me about a month and a half just to learn the structure of Sweet Home 3D, so I wanted to get feedback before I spent a lot more time on it.
- You idea of using a separate WallPoint objects for wall points seems too restrictive: why not using the same idea for rooms which corners may be bound to wall points, dimension lines bound to walls and other items? That is and excellent point, That was my exact thinking, but again, this is just a preliminary draft of the idea. For the idea of a constraint solver to be universial, then every entity would need to be point based. It would have taken me a year to get to that point in the code though. I was hoping if enough people saw value in the end goal, I could get some help with everything and make it much faster.
- Did you check whether undo / redo actions still work correctly I have re-written the undo/redo functions to make that work, and have tested them. I was not able to produce any bugs. I was not aware of JUnit, I need to learn how to use that tool for testing.
- Some javadoc comments were changed a little too fast in DimensionLine.java and maybe other places. I can look at that.
- Breaking forward compatibility of SH3D files is a difficult decision to make. Again, I am not expecting this patch to be added any time soon. If you have plans in the future for breaking that forward compatibility, then perhaps both can happen at the same time?
- Breaking existing API is not an option for the moment. Fair enough. Adding full parametric support is a big paradigm shift, so that may just be a deal breaker. Just to be clear, I want to figure out how to make it retain compatibility, but until I learn how the plugin API works, I can't say if that is possible.
Java doesn't allow overloading methods which have just a change in their return type. It was not an idea to overload, but to change the old function so that internally it calls the new function then converts to/from the old parameter/return types. So in the example there is 2 names, getArcCircleCenter() and getArcCircleCenterVector()
The lack of a Point class instead of a float array comes mainly from the origin of Sweet Home 3D: as it was first the case study of a book I wrote about Swing, I didn't want to add too many classes and chose this kind of simplification. But the good thing is that I spent more time on the application architecture which helped it to live almost 17 years so far. Might I ask why you feel that fewer classes is more simple? I mean no disrespect, so please don't take this as criticism. When you first started writing this program, computers were far less powerful and the java engine was way less effcient. So I can completely understand the desire to use fewer classes from an efficiency standpoint. But consider that Java is much more capable on modern computing systems, So maybe you mean it is more simple to maintain the code. In other words easier to read and understand what is being done in a method. If that is the case, I would argue that more classes actually increases simplification and readability. Is that not the idea of OOP?
For example take this portion of the code to join 2 walls:
public void joinSelectedWalls() { List<Selectable> selectedItems = this.home.getSelectedItems(); List<Wall> selectedWalls = Home.getWallsSubList(selectedItems); final Wall [] walls = {null, null}; for (Wall wall : selectedWalls) { if ((wall.getArcExtent() == null || wall.getArcExtent() == 0f) && (wall.getWallAtStart() == null || wall.getWallAtEnd() == null)) { if (walls [0] == null) { walls [0] = wall; } else { walls [1] = wall; break; } } } if (walls [1] == null) { Collections.sort(selectedWalls, new Comparator<Wall>() { public int compare(Wall wall1, Wall wall2) { float[] intersection1 = computeIntersection(wall1.getXStart(), wall1.getYStart(), wall1.getXEnd(), wall1.getYEnd(), walls [0].getXStart(), walls [0].getYStart(), walls [0].getXEnd(), walls [0].getYEnd()); float[] intersection2 = computeIntersection(wall2.getXStart(), wall2.getYStart(), wall2.getXEnd(), wall2.getYEnd(), walls [0].getXStart(), walls [0].getYStart(), walls [0].getXEnd(), walls [0].getYEnd()); double closestPoint1 = Math.min(Point2D.distanceSq(walls [0].getXStart(), walls [0].getYStart(), intersection1 [0], intersection1 [1]), Point2D.distanceSq(walls [0].getXEnd(), walls [0].getYEnd(), intersection1 [0], intersection1 [1])); double closestPoint2 = Math.min(Point2D.distanceSq(walls [0].getXStart(), walls [0].getYStart(), intersection2 [0], intersection2 [1]), Point2D.distanceSq(walls [0].getXEnd(), walls [0].getYEnd(), intersection2 [0], intersection2 [1])); return Double.compare(closestPoint1, closestPoint2); } }); if (walls [0] != selectedWalls.get(1)) { walls [1] = selectedWalls.get(1); } } if (walls [1] != null) { // Check parallelism 1 deg close double firstWallAngle = Math.atan2(walls [0].getYEnd() - walls [0].getYStart(), walls [0].getXEnd() - walls [0].getXStart()); double secondWallAngle = Math.atan2(walls [1].getYEnd() - walls [1].getYStart(), walls [1].getXEnd() - walls [1].getXStart()); double wallsAngle = Math.abs(firstWallAngle - secondWallAngle) % Math.PI; boolean parallel = wallsAngle <= Math.PI / 360 || (Math.PI - wallsAngle) <= Math.PI / 360; float[] joinPoint = null; if (!parallel) { joinPoint = computeIntersection(walls [0].getXStart(), walls [0].getYStart(), walls [0].getXEnd(), walls [0].getYEnd(), walls [1].getXStart(), walls [1].getYStart(), walls [1].getXEnd(), walls [1].getYEnd()); } else if (Line2D.ptLineDistSq(walls [1].getXStart(), walls [1].getYStart(), walls [1].getXEnd(), walls [1].getYEnd(), walls [0].getXStart(), walls [0].getYStart()) < 1E-2 && Line2D.ptLineDistSq(walls [1].getXStart(), walls [1].getYStart(), walls [1].getXEnd(), walls [1].getYEnd(), walls [0].getXEnd(), walls [0].getYEnd()) < 1E-2) { // Search join point for walls in the same row if (walls [1].getWallAtStart() == null ^ walls [1].getWallAtEnd() == null) { // If second wall has only one free end, join the first wall to this free end if (walls [1].getWallAtStart() == null) { joinPoint = new float [] {walls [1].getXStart(), walls [1].getYStart()}; } else { joinPoint = new float [] {walls [1].getXEnd(), walls [1].getYEnd()}; } } else if (walls [1].getWallAtStart() == null && walls [1].getWallAtEnd() == null) { double wallStartDistanceToSegment = Line2D.ptSegDistSq(walls [1].getXStart(), walls [1].getYStart(), walls [1].getXEnd(), walls [1].getYEnd(), walls [0].getXStart(), walls [0].getYStart()); double wallEndDistanceToSegment = Line2D.ptSegDistSq(walls [1].getXStart(), walls [1].getYStart(), walls [1].getXEnd(), walls [1].getYEnd(), walls [0].getXEnd(), walls [0].getYEnd()); if (wallStartDistanceToSegment > 1E-2 && wallEndDistanceToSegment > 1E-2) { // If walls don't overlap, connect first wall to the closest point if (walls [0].getWallAtEnd() != null || walls [0].getWallAtStart() == null && wallStartDistanceToSegment <= wallEndDistanceToSegment) { if (Point2D.distanceSq(walls [1].getXStart(), walls [1].getYStart(), walls [0].getXStart(), walls [0].getYStart()) < Point2D.distanceSq(walls [1].getXEnd(), walls [1].getYEnd(), walls [0].getXStart(), walls [0].getYStart())) { joinPoint = new float [] {walls [1].getXStart(), walls [1].getYStart()}; } else { joinPoint = new float [] {walls [1].getXEnd(), walls [1].getYEnd()}; } } else { if (Point2D.distanceSq(walls [1].getXStart(), walls [1].getYStart(), walls [0].getXEnd(), walls [0].getYEnd()) < Point2D.distanceSq(walls [1].getXEnd(), walls [1].getYEnd(), walls [0].getXEnd(), walls [0].getYEnd())) { joinPoint = new float [] {walls [1].getXStart(), walls [1].getYStart()}; } else { joinPoint = new float [] {walls [1].getXEnd(), walls [1].getYEnd()}; } } } } } if (joinPoint != null) { JoinedWall [] joinedWalls = JoinedWall.getJoinedWalls(Arrays.asList(walls [0], walls [1])); doJoinWalls(joinedWalls, joinPoint); this.undoSupport.postEdit(new WallsJoiningUndoableEdit(this, this.preferences, selectedItems.toArray(new Selectable [selectedItems.size()]), home.isAllLevelsSelection(), joinedWalls, joinPoint)); } } } With point and line classes, this becomes: public void joinSelectedWalls() { List<Selectable> selectedItems = this.home.getSelectedItems(); List<Wall> selectedWalls = Home.getWallsSubList(selectedItems); Vector2D joinPointLocation = null; List<WallPoint> joinPoints = null; if(selectedWalls.size() > 1) { joinPoints = WallPoint.getCommonPoints(selectedWalls); if(joinPoints.size() > 1) return; if(joinPoints.size() == 1) { if(selectedWalls.size() == 2) return; joinPointLocation = joinPoints.get(0).toVector(); } else { joinPointLocation = selectedWalls.get(0).getWallIntersectionPoint(selectedWalls.get(1)); } for (int i=0; i < selectedWalls.size(); i++ ) { Wall wall = selectedWalls.get(i); WallPoint point = wall.getClosestEndpoint(joinPointLocation); if(!joinPoints.contains(point)) joinPoints.add(point); } } if (joinPoints != null) { WallState [] preJoin = WallState.getWallStates(selectedWalls); doJoinWalls(selectedWalls, joinPoints, joinPointLocation); WallState [] postJoin = WallState.getWallStates(selectedWalls); this.undoSupport.postEdit(new WallsJoiningUndoableEdit(this, this.preferences, selectedItems.toArray(new Selectable [selectedItems.size()]), home.isAllLevelsSelection(), preJoin, joinPointLocation, postJoin)); } }
I would argue that the latter is far more simple and perhaps more efficent from a programming point as there is less math involved. Not to mention all of the references to wallAtEnd, wallAtStart, wallStartAtWallAtEnd etc. No offence but that made my head bleed trying to read it. Again, I understand why it was done, I am just not sure it is necessarily more simple.
And lastly, I do take your point of creating a fork with the possibility of reintegrating. of course I want to add to the community rather than create competing products. But perhaps this will allow developers to try out a version with incompatible API's and they can reach a consensus on whether the new features provides enough value to break compatibility, or that they want it to remain the same.
I will create a git repository for that purpose. I also noticed today that there are some changed files that are not being pulled in by SVN when I create a patch. For example, PlanController has a ton of changes that don't seem to get patched. Even if I create a patch against only that file. Meld seems to have no problem seeing the diff though. Not sure why that is. I am thinking there is a lot of code that is missing from this patch, I need to figure out why that is. Again, this patch was not intended to be included into the code, I was simply posting it for other people to look at and give feedback.
Thanks for your opinions. Cheers!
|