Download

Online

Gallery

Blog

  Index  | Recent Threads  | List Attachments  | Search
 Welcome Guest  |  Register  |  Login
Login Name  Password
 

Sweet Home 3D Forum



No member browsing this thread
Thread Status: Active
Total posts in this thread: 12
Posts: 12   Pages: 2   [ Previous Page | 1 2 ]
[ Jump to Last Post ]
Post new Thread
Author
Previous Thread This topic has been viewed 3405 times and has 11 replies Next Thread
nathanjshaffer
Newbie



United States
Joined: Dec 20, 2020
Post Count: 6
Status: Offline
Reply to this Post  Reply with Quote 
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!
[Aug 6, 2022, 2:11:15 PM] Show Printable Version of Post    View Member Profile    Send Private Message [Link] Report threatening or abusive post: please login first  Go to top 
thkoch
Newbie




Joined: Mar 25, 2023
Post Count: 1
Status: Offline
Reply to this Post  Reply with Quote 
Re: Patch submission

Hi Nathan,

are you still interested to go ahead with the Git repository? I believe it would be very beneficial for sweethome3d if code contributions would be easier. Unfortunately there's a vicious circle:

1. Puybaret does not see any benefit in Git compared to SVN.
2. SVN actually has the advantage, that you can commit your dependencies to a lib/ folder without worrying about the repository size.
3. The advantages of Git only become visible when multiple people contribute to the project.
4. People hesitate to contribute to a project that still uses SVN in 2023.

A friendly Git fork of the project could attract contributors and show the advantages of Git to Puybaret. The Subversion repo could be maintained as the master branch of the Git repo and pull requests could be reviewed and forwarded to Sourceforge.

With time, Puybaret might change his mind.

I'd be happy to help with such a friendly Git fork.

To make this work, Puybaret would at least need to accept a patch that adds an ivy.xml to the repo and the lib/ folder needs to be ignored when creating the svn2git repo.
[Mar 26, 2023, 3:51:58 PM] Show Printable Version of Post    View Member Profile    Send Private Message [Link] Report threatening or abusive post: please login first  Go to top 
Posts: 12   Pages: 2   [ Previous Page | 1 2 ]
[ Jump to Last Post ]
Show Printable Version of Thread  Post new Thread

    Get Sweet Home 3D at SourceForge.net. Fast, secure and Free Open Source software downloads
   
© Copyright 2006-2024 eTeks - All rights reserved