get/set WorldPosition/WorldOrientation/WorldScale

[Beauty]

Hi,

I miss the functions getWorldPosition(), getWorldOrientation() and getWorldScale().


Until Ogre 1.5 the SceneNode class contained the properties getWorldPosition(), getWorldOrientation() and getWorldScale().
Unfortunately in the Ogre core library the methods getWorldPosition, setWorldPosition, getWorldOrientation, setWorldOrientation were removed.
Now we have to use _getDerivedPosition(), _setDerivedPosition(), _getDerivedOrientation() and _setDerivedOrientation() instead.

I don't know why the Ogre maintainers removed the world members, because they wrote application developers shouldn't use members which starts with underscore. These members would only be for internal stuff. Now they force Ogre users to use such methods. Maybe they wanted to remove redundant methods? They didn't gave me an answer for this question which I asked in the main forum.

I think it would be nice to have World paramters again. Everybody knows immediately what's its meaning.

This is my tiny personal feedback.

[Beauty]

Here I added this proposal to the bug tracker:
http://www.ogre3d.org/mantis/view.php?id=382

Suggestion:
Add the cathegory called proposal to the bug tracker.

Reporting papercuts
Please report any papercut you encountered in our bugtracker. Create a new report and prefix the title with “[Papercut]“. If you feel there is need for a discussion about a proposed change, then open a thread in this forum and link to the corresponding report in the tracker.

This bug tracker I never used. There I wanted to add an entry, but I was confused and didn't understand how to do (even after log-in).
After some clicking I found it out.
So it would be good to add a note how to do.

[Kojack]

The only problem with the getWorld style (which I prefer, I don't like the _getDerived version) is that the node's _getDerived methods are inherited by bones, but calling them on a bone doesn't resolve to a world coordinate (position, orientation or scale) but to a value local to the scene node the skeleton is in. So replacing them with getWorld would mean it doesn't really get the world value when dealing with skeletons (but it would be ok for nodes and scenenodes).

At the very least the underscore should be removed. I'd prefer a getWorld method than a getDerived method, but it would require special handling in bones (it would be good to have true getWorld methods in bones which understand that skeletons are within scenenodes, but it would make the code more complex than just a rename).

[Beauty]

Interesting to know the reason why the name was changed in the past.

Maybe we can use World functions in the Node class, again.

Proposal:

When the Bone class inherits from it, there could be done this:
* Add Derived functions (without underscore)
* Override the World functions. They should return the same result like Derived, but additionally add a log message to the ogre.log file.

Example log entry: WARNING: Don't use Bone.getWorldPosition(). Use Bone.getDerivedPosition() instead.

So the application doesn't break when somebody uses the World params of a bone. But the developer has a good chance to see the note.
More clear would be to remove the World params from the Bone class, but I suppose this isn't possible.


The same problem I saw in some inherited .NET classes.
The solution: Useless inherited functions throws a special exception.
This behaviour is also documented in the class and function descriptions.

[CABAListic]

sinbad's reasoning for the underscore (and removal of getWorld...) is found in this thread: http://www.ogre3d.org/forums/viewtopic.php?p=221113

I think it's still valid. If we were to reintroduce them, they should probably behave differently, which would make them more complex and slower. As such it might be out of the scope of a papercut.

[jacmoe]

Thanks, now I can relax: I just knew that we had that discussion before.
The underscore here means *caution* rather than don't use.

[Beauty]

Thanks, now I know.
I didn't see this topic before and when I asked some time ago in a thread I got no answer.

The reason for the "warning" symbol seems to be important in some cases.
So I added it to the wiki page -SceneNode.

Unfortunately this note isn't available in the class documentation.
So it would be good to add it to the description of SceneNode._getDerivedPosition(), _getDerivedOrientation() and _getDerivedScale().
Also for other classes which contains these functions (e.g. Bones).

[CABAListic]

You are right, though, getting the world position of a SceneNode is an important and imho necessary feature, and using the _getDerived methods may confuse people, particularly about the subtleties of the lazy update involved (although in most cases this will not be an actual issue).

I haven't looked at the SceneNode update code, so maybe this suggestion doesn't make sense. But I could imagine readding those getWorld... functions taken an optional parameter (bool forceUpdate = false) which, if enabled, will force the SceneNode hierarchy to update if necessary.

[Jabberwocky]

Here's one possible solution:

Code: Select all

virtual const Vector3 & getWorldPosition( bool forceUpdate = false );

There should be some clear header comments as well, something like this:

Code: Select all

// Discuss how nodes in the hierarchy are lazily updated.
// Discuss the forceUpdate parameter, which can be set to true to force an update 
//      such that we consider any changes which have been made to our parent nodes.
// Discuss/warn about performance of using forceUpdate = true
virtual const Vector3 & getWorldPosition( bool forceUpdate = false );

I'm relatively indifferent to whether we use getWorldPosition or getDerivedPosition. But by adding a forceUpdate parameter, I think we can lose the _ on the function name, since this parameter makes the lazy update very explicit. It also gives the caller more control to actually get an updated position, at the cost of some performance. The false default value for the forceUpdate parameter would make this new function behave exactly the same as the old _getDerivedPosition function (unless forceUpdate is explicitly set to true).

The only issue is we'd have to lose the const from the current function:

Code: Select all

virtual const Vector3 & _getDerivedPosition(void) const;

I don't know how often Ogre or it's users pass around const Nodes*. Maybe we could leave the old _getDerivedPosition around for backwards compatibility.

[Beauty]

Thanks for your idea

Maybe we could leave the old _getDerivedPosition around for backwards compatibility.

My personal opinion:
I would prefer to remove _getDerivedXxx() when we add getWorldXxx(), because redundant methods aren't very good for a clear API.
When somebody updates his application to a newer Ogre version, it's normal that he need to do some changes. Important is to document this on a public place. (e.g. the change log)
The advantage of a clear API (without unnecessary redundancies) is the long-term usability.
New users (or even old ones) could be confused about and don't know the differences.

[jacmoe]

It's only common developer courtesy to not break a lot of peoples code and flag functions as deprecated for a while before removing them. Especially such a widely used function.
Don't underestimate new users. Or existing users with a codebase which breaks.

[madmarx]

While getWorldTransform() functions are interesting, there is a use for the skeleton hiearchy too. When I do IK on characters, I prefer to do it in the skeleton space rather than in the world space. Currently, _getDerivedXXX is one way to do that (and could have been called _getHierachyXXX). Therefore, I am not convinced that they should be suppressed (even if the getWorldXXX are added).

[Beauty]

It's only common developer courtesy to not break a lot of peoples code and flag functions as deprecated for a while before removing them. Especially such a widely used function.

Related to the removed getWorldXxx() functions: I never did read a "deprecated" note before I used the Ogre version where is was removed. (Maybe a note was in the class reference, but I didn't read the related method description, when I did know what's its task.)
Well, it doesn't matter if there was a note for getWorldXxx() methods or not.
I just want to agree to jacmoe: It's a good practice to say it's deprecated before removing it.

[Beauty]

I proposed to add remarks for the "danger" of using the _derived methods.
Also some more details (what happens?) could be published in the Ogre class reference.

If you like to contribute "text snippets", use the topic Add description of class members to Ogre class reference.

[syedhs]

I don't think renaming back _set/getDerivedXXX back to set/getWorldXXX functions are going to be 'problematic' because user can spot the compilation error and just do a global search and replace - done!

What need (must) to be taken care is the function behavior - it should behave the same across the version. Otherwise it is really harmful. Because all you do is upgrade to newer version, no code changes and then, some features which had been working perfectly simply stop working. I personally feel setWorldXXX is a better name for what it does

[Beauty]

As I read, Ogre 1.8 should be published official soon. So I looked for the state of this papercut topic.
In the papercut tracker this topic has still the status "open" and the last post is some months ago. So I want to pull it up for the case that we want to change something before release of Ogre 1.8.

CABAListic had a nice proposal:

You are right, though, getting the world position of a SceneNode is an important and imho necessary feature, and using the _getDerived methods may confuse people, particularly about the subtleties of the lazy update involved (although in most cases this will not be an actual issue).

I haven't looked at the SceneNode update code, so maybe this suggestion doesn't make sense. But I could imagine readding those getWorld... functions taken an optional parameter (bool forceUpdate = false) which, if enabled, will force the SceneNode hierarchy to update if necessary.

Here's one possible solution:

Code: Select all

virtual const Vector3 & getWorldPosition( bool forceUpdate = false );

There should be some clear header comments as well, something like this:

Code: Select all

// Discuss how nodes in the hierarchy are lazily updated.
// Discuss the forceUpdate parameter, which can be set to true to force an update 
//      such that we consider any changes which have been made to our parent nodes.
// Discuss/warn about performance of using forceUpdate = true
virtual const Vector3 & getWorldPosition( bool forceUpdate = false );

I suggested to add notes about the update effort to the Ogre API description.
It would be nice, if somebody would do it. (Or if somebody has no write access to the repository, he could write a description as template.)
Here are related quotes:

Unfortunately this note isn't available in the class documentation.
So it would be good to add it to the description of SceneNode._getDerivedPosition(), _getDerivedOrientation() and _getDerivedScale().
Also for other classes which contains these functions (e.g. Bones).

The issue here is that derived positions are lazy-updated for speed, not every time you make a change to nodes in the hierarchy. Thus the result may not be accurate unless SceneManager::updateSceneGraph has happened, or you force an update for a sub-branch of the tree. When you make a change to a parent node, the update flag is not propagated all the way down the tree (for the same efficiency reasons), it's just flagged as pending a cascade which is propagated later. Thus a node several children down may have no knowledge that it is currently out of date. Within 1 level of the tree it's ok, but beyond that it's not.

Without this lazy update approach, making lots of updates to scene objects in deep hierarchies becomes very slow. Thus we mark the _getDerived methods with an underscore because grabbing derived updates all the time can be both costly and /or inaccurate depending how you propagate changes. Scene managers can rely on them because updateSceneGraph has resolved all the changes.