Jan 21, 2026, 12:44:31 AM
|
Sweet Home 3D Forum » List all forums » » Forum: Developers » » » Thread: Patch submission » » » » Post: Re: Patch submission |
Print at Jan 21, 2026, 12:44:31 AM |
| 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 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.public Vector2D getArcCircleCenter(){...}could be public Vector2D getArcCircleCenterVector(){...}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 |
|
|
Current timezone is GMT Jan 21, 2026, 12:44:31 AM |