Setter and Getter Methods for Shapes (Part 1)

I've got a patch (the first of 2) ready to go adding getXXX and setXXX methods to the shape classes. This one covers the polyhedrons. It also adds a class RegularPolyhedron which factors out the common sideLength property that all of these classes share.



It's fairly long as it impacts half a dozen classes so rather than pasting it in here you can get it from http://ianp.org/2008/12/jme-regular-polyhedron.patch.



I'll leave it for 48 hours or so before committing it to give people time to review it, and I'll try to get the other patch covering the rest of the shapes put up today or tomorrow for review.

Perhaps the update method in RegularPolyhedron should be named something a little more descriptive and not already associated with something else. I suggest recompute, or recomputeGeometry…



Also, what is the use case for needing to change the side length? I remember discussions just this weekend about why getters and setters on the geometry shapes are not necessarily a good idea.

nymon said:

Also, what is the use case for needing to change the side length? I remember discussions just this weekend about why getters and setters on the geometry shapes are not necessarily a good idea.

I made an argument against conventional field getters which would merely return the initialization values, but the way it is implemented now I think both getters and setters will work as expected.
I support the statement about renaming the update method, other than that the patch looks fine and as a bonus even increases the "OO-ness" of the ployhedron classes ;)

Well, one use case for getters/setters would be to ease the use of scene graph shapes in modelling tools like MW3D, and for that the performance hit wouldn't be an issue either.



I think that recomputeGeometry is definitely a better name than update as it clearly describes what the method is supposed to do, I'll change the code to use that.



Also, some of the other shape classes have differing names for this method (there are at least 3 different names used). I'd like to make the name recomuteGeometry standard across all of the shapes, for now I'll leave the other methods in place as well but maybe mark them as deprecated. Will post an updated patch (and set of patched classes for easy review) in a short while.

ianp said:

Also, some of the other shape classes have differing names for this method (there are at least 3 different names used). I'd like to make the name recomuteGeometry standard across all of the shapes, for now I'll leave the other methods in place as well but maybe mark them as deprecated. Will post an updated patch (and set of patched classes for easy review) in a short while.

Yes, I'm all for consistency. Post a new topic.

plus 1…

Cool. The shapes as they stand now are at http://ianp.org/2008/12/jme-shapes-2.tgz for your review. I'll start a new topic to discuss removing the existing setters.



Having thought about this some more I'm starting to think that I now agree with the arguments that nymon put forward and that maybe it's better not to have individual property setters on the shape classes. I still think that the getters are useful, and that it would be a good idea to standardise the name of the method used to update these (to recomputeGeometry). These files reflect that.



@nymon: I bow to your superior wisdom :smiley:

Committed (get methods only).

The resize call in the updateGeometry that replaced initialize break my game!

It got my quad of HUD 2x sized.

What about it?

At really thats not the Quad, im investigating yet what update that breaks.

Actually it happens on Mac only.

Im investigating some more.

clovis:  what do your last 3 posts have in common with getters for geometry??  (I am very confused by those posts…)



Did anyone actually get a chance to verify/diff this patch and see what the impact was going to be??  Looking at it briefly it seems to have a LOT going on, unfortunately because it is not formatted anymore it is VERY difficult to read.  I did NOT verify or even check it since it looked like it was covered…

you can check the diff here more easily:

http://code.google.com/p/jmonkeyengine/source/detail?r=4075



clovis means this change: http://code.google.com/p/jmonkeyengine/source/diff?spec=svn4075&r=4075&format=side&path=/trunk/src/com/jme/scene/shape/Quad.java



But i don't see any change in the behavior, only clean up and unification.

sorry, i had weird behaviour on quads when i updated.

But i have discovered that its not about these mods.

Sorry for that!

These fix are ok.

I need to test a little more on osx to get exactly where is the problem.

Until that, ignore my posts please.

Thanx core-dump, my mind is at ease (and I learned something :))



Good additions ianp.

Thanks!  :smiley: