Skip to content

Conversation

@stephengold
Copy link
Member

Now that issue #1119 is closed, protect some of the public no-arg constructors that are intended only for use by JME's serialization system (SavableClassUtil).

@stephengold stephengold merged commit 9d2d393 into jMonkeyEngine:master Dec 1, 2019
@stephengold stephengold added this to the v3.3.0 milestone Dec 1, 2019
@pspeed42
Copy link
Contributor

pspeed42 commented Dec 2, 2019

Some of these I'm not so sure about. In the cases like InstancedGeometry the only difference between the no-arg and String-arg constructor is that the geometry won't have a name.

Furthermore, things like LightNode and CameraNode, it's sometimes nice to be able to set the light and camera later after construction... but especially for CameraNode. (Though an argument can be made for simply deprecating these classes in favor of the controls.)

@stephengold
Copy link
Member Author

The javadocs for InstancedGeometry, LightNode, and CameraNode state that the no-arg constructors should not be used.

@pspeed42
Copy link
Contributor

pspeed42 commented Dec 2, 2019

But at least in the case of InstancedGeometry, it's wrong. The name should never be required. I suspect that many of these comments were just cut-pasted or typed without thought.

@pspeed42
Copy link
Contributor

pspeed42 commented Dec 2, 2019

Note: it 100% does make sense to make protected the constructors that will make "bad objects"... so I'm completely on board with 90% of these changes as there is no possible way that a user could be using these correctly. But the three cases I mention could be working fine for them... and definitely InstancedGeometry. For example, I almost never use the name-based constructors myself because I couldn't care less about the names of my spatials.

@stephengold
Copy link
Member Author

I'll do a partial revert, then, publicizing those 3 constructors and removing the misleading javadocs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants