Sweet Home 3D Forum
»  List all forums
» »  Forum: Developers
» » »  Thread: Patch submission
Print at Jan 21, 2026, 2:33:12 AM
Posted by nathanjshaffer at Aug 1, 2022, 4:14:12 PM
Patch submission
I'm not sure where is the best place to submit a patch. I have been working on a modification that I think will be useful. This program would benefit greatly from a constraint solver. This would allow for things like locking walls to a certain length/angle or having dimensions that are attached to walls and grow with the wall.

To work towards this, I have made a pretty deep rewrite that does a couple things in this patch.

1. float arrays have been replaced by a Vector2D class. This allows for simpler code and puts a lot of the point math into a library. This will also be important for implementing a constraint solver as the algorithm should deal with points, not lines.

2. walls ends are connected to a wallpoint instead of directly to a wall. Moving a point, moves the connected walls. This allows more than 2 walls to be joined at a point.

3. I made the dimension help lines appear during dragging a wall. This helps with better placement based on wall length

This patch allowed me to learn how the code was laid out and to simplify a lot of things.. I don't see an file upload, so I will just add it as a post.

Posted by Puybaret at Aug 1, 2022, 5:09:17 PM
Re: Patch submission
Thanks for trying to improve Sweet Home 3D. You may submit your patch in a Features request ticket.
----------------------------------------
Emmanuel Puybaret, Sweet Home 3D creator

Posted by sjb007 at Aug 1, 2022, 11:09:40 PM
Re: Patch submission
That sounds like some really interesting stuff, Nathan. I look forward to taking it for a spin. In the meantime, you told us the benefits... but, is anything lost with your patch? Any functions that will have to be reimplmented?

Posted by nathanjshaffer at Aug 2, 2022, 7:13:01 PM
Re: Patch submission
Patch submitted - ticket #1090

No functionality lost as far as I am aware. I am still working on making the new version save/load and will need to be able to update older saved files to the new version. I also didn't completely replace all of the float arrays in the entire codebase. It was a lot of time going through it all, so i created a conversion function to shortcut as I need to simply get it to build and test. so some of the classes that are completely unrelated to walls still use float[]. At some point I will get around to doing everything. this was more of a proof of concept.

I am researching constraint solver algorithms. I found a university dissertation where someone had written a simple 2D solver in java. I contacted her to see if I could get the source code, but she no longer has it unfortunately. The paper is
here and has some sample Java classes as well as psuedo-code for the pan algorithm. It will take some doing to get it all figured out, but should make this project far more capable.

Posted by Puybaret at Aug 3, 2022, 1:27:37 PM
Re: Patch submission
Thanks for posting this patch. I will have a look to it but I’m a little worried by the changes it may apply to the Sweet Home 3D API. If these changes are not compatible with the existing API (for example you changed the return type of methods which returned à float array), existing plug-ins and derived versions won’t work anymore.
----------------------------------------
Emmanuel Puybaret, Sweet Home 3D creator

Posted by nathanjshaffer at Aug 4, 2022, 12:44:01 AM
Re: Patch submission
That's a valid concern. Is the API what plugins use? Do API developers access these finctions directly, or does the API act as a layer? I wrote toFloatArray() / fromFloatArray functions in the Vector2D class that convert betweenn the two as there are same java built-in libraries that use float arrays, so that would help to transition any plugins that need to old return values. Anther option is to keep the old methods and mark them obsolete, then write new functions with a different name with the new return type. This would just require some new thinking about what the new method names would be since the old names were the best option.

for example
public Vector2D getArcCircleCenter(){...}


could be
public Vector2D getArcCircleCenterVector(){...}

/*
* @deprecated
*/
public float[] getArcCircleCenter(){
return Vector2D.toFloatArray(getArcCircleCenterVector());
}


this would allow any old plugins to keep working

Posted by nathanjshaffer at Aug 4, 2022, 12:56:00 AM
Re: Patch submission
Also, I just posted a second patch with save/load completed for vectors. Still need to make it convert the old saved files.

Posted by Daniels118 at Aug 4, 2022, 10:35:11 AM
Re: Patch submission
@nathanjshaffer
Although it would be nice to have a class to represent a point instead of an array, I think making a change in the existing API is a bit bully towards the plugin developers.
You could consider to implement the new features as a plugin, and if you need the points as Vector2D instead of float arrays you can just convert them forth and back.

Posted by nathanjshaffer at Aug 4, 2022, 5:32:23 PM
Re: Patch submission
I get that, but my thought is to actually make an underlying change to the 2d system. Basically to make the program behave more like a modern CAD system with a fully constraint based design. This is a step in that direction. It's not just changing float[] to vector2D for convenience, it is changing how walls are stored and joined. in the old system, each wall kept a reference to the wall connected to it. With the new system, each wall is connected to a point. Moving the point, moves the walls connected. This will open up a lot of possibilites, such as having multiple walls joined together, (already implemented) having walls join to a wall midpoint, or to a coincident, locking angles and wall lengths, keeping walls parallel or perpendicular, locking furniture and window spacing, etc. Adding a constraint solver as a plugin doesn't make much sense to me as that functionality, if included should really be a part of the base programming. Converting everything back and forth and constraint solving every time a wall is drawn or dragged would be computationally expensive. Further, as I mentioned, now that I am aware of the potential conflicts with plugin developers, I think it is perfectly possible to provide reverse compatibility with most of the functions through deprecated methods to ease the transition. Also Vector based classes will make plugin developers jobs much easier. As long as they have the choice and nothing is forced on them, it shouldn't be too burdensome.

The codebase is 15 years old, and while I am not advocating change for the sake of change, it might be naive to think a growing project won't have to rethink some major changes in the architecture over that time. My suggestion is to create a branch to develop a constraint based system and try to get it working to see if there is interest from the plugin developers. If there is then, we can work on a migration solution so their plugin's don't break. I have zero interest on shitting on plugin developers, so I am completely open to helping work out a solution that makes everyone happy

Posted by Puybaret at Aug 5, 2022, 1:14:17 PM
Re: Patch submission
I had a look to your patch. Here are my 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 (their implementation may depend on such classes but it's another point). It was an intuition to program this way, but I think I was right when I had to port Sweet Home 3D to JavaScript where java.awt.geom package doesn't exist. Your new classes Vector2D and Line2D introduce a change in this way of programming because they inherit from classes of java.awt.geom package.
Inheriting of java.awt.geom.Point2D which has some public fields don't respect also the conventions of the program where all fields are encapsulated.
- Naming of Vector2D is questionnable since it's a point not a vector.
- You seem to have not applied standard conventions to your code (spaces, no if else for statements without braces, no if and its consequence on the same line...). If you want your code to end one day in Sweet Home 3D code base, you should use at minimum the Source > Format menu item in Eclipse (maybe current rules should be updated because they don't seem to add braces).
- 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?
- Did you check whether undo / redo actions still work correctly (probably running JUnit tests will be enough for that)?
- Some javadoc comments were changed a little too fast in DimensionLine.java and maybe other places. It also seems that you copied some code from elsewhere without updating comments (like @since JavaFX 8.0 which probably comes from Point2D class in JavaFX).
- Breaking forward compatibility of SH3D files is a difficult decision to make. You may still open a 7.0 SH3D file with Sweet Home 3D 1.0, even if such a wide support isn't really interesting because all the missing features will be ignored by this old version.
Breaking this wide compatibility will happen sooner or later, because it doesn't worth keeping the Home serialized version entry and Home.xml entry in a SH3D file for ever. But I've prepared this change for a long time since SH3X is a file extension associated to Sweet Home 3D since version 5.3 when Home.xml entry was added.
- Breaking existing API is not an option for the moment. You may say that after 15 years, it's time to break things, but I fear that developers won't follow you, arguing that continuing these 15 years of compatibility is nice for them. By the way, it's not in Java spirit, and not in mine too.

for example
public Vector2D getArcCircleCenter(){...}


could be
public Vector2D getArcCircleCenterVector(){...}

/*
* @deprecated
*/
public float[] getArcCircleCenter(){
return Vector2D.toFloatArray(getArcCircleCenterVector());
}

Java doesn't allow overloading methods which have just a change in their return type. You'll have to use a different name for such methods.

Although it would be nice to have a class to represent a point instead of an array, I think making a change in the existing API is a bit bully towards the plugin developers.
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.

But let's finish on a positive point: the good thing of the GNU GPL is that it allows anyone to create derived versions and redistribute them, so feel free Nathan to continue your development and propose some improvements which will add some must-have features.
----------------------------------------
Emmanuel Puybaret, Sweet Home 3D creator

Posted by nathanjshaffer at Aug 6, 2022, 4:11:15 PM
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!

Posted by thkoch at Mar 26, 2023, 5:51:58 PM
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.


RSS feeds
Powered by mvnForum 1.2.2 GA
© Copyrights 2002-2010 by MyVietnam.net
Current timezone is GMT
Jan 21, 2026, 2:33:12 AM