Some coming changes

[Martin Scheffler]

Hi all,

some smaller changes, please speak up if you have objections:

* Will remove CharProperty. This is not used anywhere.
* Will change VecXProperty to store values as C arrays, not osg::VecX
objects. This will not break the interface and will make interaction
with middleware easier
* Will rework some entity systems to use dynamic properties instead of
relying on OnPropertyChanged. If possible, I want to deprecate
OnPropertyChanged, users should use dynamic properties instead.

Cheers,
Martin

[Riccardo Corsi]

Hi Martin,

I'd like to share some thoughts:

1. Changing the osg::VecX into a C array would require a cast between the two, not to copy values around. Even though the memory footprint should be the same, are you 100% sure that we can make that assumption for all the platforms?

2. Regarding the OnPropertyChanged(), what you suggest is basically that the method will disappear and instead a Dynamic Prop should be used whenever users needs to be notified about a prop change. And thus implement any reaction in the Get/Set functor instead of deriving OnPropertyChanged(). Is that correct? What about the Finished() method, will it remain there?

3. This point is not about the changes you mention here, but about PropertyContainer now deriving from GroupProperty - sorry I didn't react before. 

I understand that it helped to remove a lot of duplicated code, but conceptually I don't like very much the fact that every container ISA Property itself. In particular a Component is now a Property as well, even though it represents something quite different. 

What do you think about this?

Thank you,

cheers. 

Ricky

[Martin Scheffler]

2012/6/5 Riccardo Corsi <riccard...@kairos3d.it>:

> Hi Martin,
>
> I'd like to share some thoughts:
>
> 1. Changing the osg::VecX into a C array would require a cast between the
> two, not to copy values around. Even though the memory footprint should be
> the same, are you 100% sure that we can make that assumption for all the
> platforms?
>

Well, at the moment the return value of VecXProperty::Get() is
actually passed by copy, so performance could be improved by returning
a const reference.

http://en.wikipedia.org/wiki/C%2B%2B_classes#cite_note-C.2B.2B03_9.2.2F17-7
A pointer to a POD-struct object, suitably converted using a
reinterpret cast, points to its initial member and vice versa,
implying that there is no padding at the beginning of a POD-struct.[8]
So I believe reinterpret_casting between float[3] and osg::Vec3 should
be well.defined. I wrote those tests, they work for me:

TEST(ReinterpretCastsV3f)
{
float values[3] = {3.0f, -234.234f, 0.0f};
const osg::Vec3f& casted = reinterpret_cast<const osg::Vec3f&>(values);
CHECK_EQUAL(values[0], casted[0]);
CHECK_EQUAL(values[1], casted[1]);
CHECK_EQUAL(values[2], casted[2]);
}

TEST(ReinterpretCastsV4d)
{
double values[4] = {3.0f, -234.234f, 0.0f, 3254325.235325};
const osg::Vec4d& casted = reinterpret_cast<const osg::Vec4d&>(values);
CHECK_EQUAL(values[0], casted[0]);
CHECK_EQUAL(values[1], casted[1]);
CHECK_EQUAL(values[2], casted[2]);
CHECK_EQUAL(values[3], casted[3]);
}

TEST(ReinterpretCastsQuat)
{
double values[4] = {3.0f, -234.234f, 0.0f, 3254325.235325};
const osg::Quat& casted = reinterpret_cast<const osg::Quat&>(values);
CHECK_EQUAL(values[0], casted[0]);
CHECK_EQUAL(values[1], casted[1]);
CHECK_EQUAL(values[2], casted[2]);
CHECK_EQUAL(values[3], casted[3]);

}

> 2. Regarding the OnPropertyChanged(), what you suggest is basically that the
> method will disappear and instead a Dynamic Prop should be used whenever
> users needs to be notified about a prop change. And thus implement any
> reaction in the Get/Set functor instead of deriving OnPropertyChanged(). Is
> that correct? What about the Finished() method, will it remain there?

Correct. I am not yet sure if this will work, especially for
JavaScript properties.
The Finished() method has to stay so that the component can react to
changes in multple interdepenent properties.

> 3. This point is not about the changes you mention here, but about
> PropertyContainer now deriving from GroupProperty - sorry I didn't react
> before.
> I understand that it helped to remove a lot of duplicated code, but
> conceptually I don't like very much the fact that every container ISA
> Property itself. In particular a Component is now a Property as well, even
> though it represents something quite different.
> What do you think about this?
>

Hmm yes, a has-a relation would maybe be better than an is-a relation.
If you want you can try changing it, you will probably have to forward
most methods of GroupProperty through Component

Cheers,
Martin

[Riccardo Corsi]

Hi Martin,

I'll reply inline below

I didn't notice that currently VecX props did return a copy of the vector they hold... 

so even coping values from the POD-struct to the VecX wouldn't cause performance drop with respect to the current version. 

I don't know , it's probably better to keep a safe version which does the copy, and use the reinterpret cast in performance critical cases.

How do you see that?

 

> 2. Regarding the OnPropertyChanged(), what you suggest is basically that the
> method will disappear and instead a Dynamic Prop should be used whenever
> users needs to be notified about a prop change. And thus implement any
> reaction in the Get/Set functor instead of deriving OnPropertyChanged(). Is
> that correct? What about the Finished() method, will it remain there?

Correct. I am not yet sure if this will work, especially for
JavaScript properties.
The Finished() method has to stay so that the component can react to
changes in multple interdepenent properties.

Right, if you understand that this works on the javascript side, I agree on the change.

 

> 3. This point is not about the changes you mention here, but about
> PropertyContainer now deriving from GroupProperty - sorry I didn't react
> before.
> I understand that it helped to remove a lot of duplicated code, but
> conceptually I don't like very much the fact that every container ISA
> Property itself. In particular a Component is now a Property as well, even
> though it represents something quite different.
> What do you think about this?
>

Hmm yes, a has-a relation would maybe be better than an is-a relation.
If you want you can try changing it, you will probably have to forward
most methods of GroupProperty through Component

Good, I'll try to change the code from ISA to HAS-A, but I'm stuck on a project for some days...

Thanks,

Ricky

[Peter C.]

I've been doing some thinking and I may see a problem with removing CharProperty. Some APIs and Specs (such as DIS) use Char values to store data. Removing the CharProperty may have negative impacts in implementing such functionality.

[Martin Scheffler]

Actually DIS was why I added it in the first place. But chars can be
stored just as good as ints - I store all 7 digits as a string
separated by spaces. Also, messages can directly hold char members
that are not properties, you just have to override the clone method to
make sure they are copied.
I just wanted to get rid of the char property because you somewhere
have to draw the line between flexibility and code complexity.

Cheers,
Martin

2012/6/19 Peter C. <th3f...@gmail.com>:

[Peter C.]

I think I found an issue in dtEntity as a result of removing the Char type: On setting the dtEntity build options to use strings instead of StringIDs when compiling using Visual Studio 2010, running the editor returns the error in the attached image. This appears to be a loose reference somewhere to the char property that got removed.

Thanks,
Peter

[Martin Scheffler]

Hmm, can't reproduce, and I don't really think this is caused by
removal of the char property.
Can you please rebuild from scratch (maybe also clean cmake cache) and
report if the problem still pops up?

Cheers,
Martin

2012/6/21 Peter C. <th3f...@gmail.com>:

[Peter C.]

Alright, will do, I've also been tracking down some issues with the editor as well. It appears as though the editor has memory allocation/access violation issues on 64 bit platforms when resetting the scene. It breaks on the thread queue, however I'm still trying to find the problem. Maybe clearing everything out may help.


Cheers,
Peter

[Peter C.]

I've been rebuilding QT for a while (it takes like a day to build), as I had some QT issues. I am apparently getting pointer issues that may or may not be connected to v8. I should also note that I have ran PVS Studio against dtEntity and OpenSceneGraph, OSG is coming up with thousands of issues in PVS Studio, mostly related to 64 bit support, and I have yet to run against v8, however the v8 includes aren't exactly promising when PVS Studio parses them. Most of the issues I have gotten is from 32 bit types being passed around with memsize types, which is a recipe for buffer overflows (or access violations), and judging by the crashes, I would say that's exactly what is happening.My current build environment is a 64 bit build using Visual Studio 2010, and most of the errors I am getting in PVS Studio stem from 64 bit memory allocation issues. Over the next few weeks, if I'm not focusing on school or other stuff, I'm going to try to fix some of these memory allocation issues and toss them back to you, OSG, V8, and QT. I would like to request permission however to base some implementations to fix the memory allocation issues on your properties implementation. For the most part, the properties types in dtEntity don't hit anything on PVS Studio unless you are trying to interface with something external.

For reference:
http://en.wikipedia.org/wiki/Buffer_overflow
http://www.viva64.com/en/l/


Thanks,
Peter

[Peter C.]

Alright, I think I may have been looking in the wrong places. I ran the debugger on Visual Studio more, and I'm thinking it has to do with refreshing the scene while empty, so a null pointer is causing issues somewhere. I'm going to do some more research in to this, it may or may not be a 64 bit issue after all. I'm going to check with Valgrind and see if I can find any hints.

[Martin Scheffler]

Hi Peter,
we are running dtEntity/v8/osg on 64 bit linux without any issues.
It is entirely possible that there are crashes in the editor. I
somewhat hacked it together quickly from existing Qt components, and
it is not used by us in any projects in this form.
I'm gonna do some testing with clearing the scene, maybe we also get
crashes but never encountered them because we don't have the clear
functionality in other programs

Can you send me the output of PVS studio for dtEntity? Do you have a
trial version or full? It's always nice to have static testing

Cheers,
Martin

2012/6/24 Peter C. <th3f...@gmail.com>:

[Peter C.]

I have a temporary license for PVS Studio for Open Source dev. I wrote them and asked if they had an open source/academic license, and they offered me a free 2 month license. I'll get the output for OSG and dtEntity together in a little bit, I've been up for about 8 hours trying to track down bad pointer, buffer overflow, and memory allocation errors while running dtEntity.

[Martin Scheffler]

Hi Peter,
you said there were crashes when resetting the scene. Do you get
crashes when you choose "new scene" in the editor or "reset system"?
"Reset system" caused crashes for me as well. I never got around to
fully implementing that function, so I threw it out for now. The idea
was to kill all entities, unload all javascripts, remove all cameras
etc. This has to be carefully executed of course, but as I never
actually needed that feature I never really got around to cleanly
implementing it.

Cheers,
Martin

2012/6/24 Peter C. <th3f...@gmail.com>:

[Peter C.]

Alright, understood, I was using "reset system". I've been taking some time off lately to cool down and try to get back some sanity after that long debugging session. Since patching to the latest SVN revision and running the editor however, the editor motion component appears to be broken, I am getting tons of this message when running the boids examples:

"
File: Scripts/osgveclib.js Line: 994 Message:
        result[0] = a[1]*b[2]-a[2]*b[1];
                          ^TypeError: Cannot read property '2' of undefined
    at Object.cross (Scripts/osgveclib.js:994:27)
    at EditorMotionComponent.update (Scripts/editormotionmodel.js:232:16)
    at Function.<anonymous> (Scripts/editormotionmodel.js:364:21)
    at __triggerIntervalCBs (Scripts/stdlib.js:88:18)
    at Function.__executeTimeOuts (Scripts/stdlib.js:101:3)

"
Cheers,
Peter

[Peter C.]

And now after a clean checkout and rebuild it is doing this on middle click in the boids scene:
"
File: Scripts/osgveclib.js Line: 1054 Message:
        r[0] = a[0]-b[0];
                     ^TypeError: Cannot read property '0' of undefined
    at Object.sub (Scripts/osgveclib.js:1054:22)
    at EditorMotionComponent.mouseMove (Scripts/editormotionmodel.js:189:39)

[Martin Scheffler]

Hi Peter,
yupp, that was brocken. I changed the way the Screen.pickEntity
function returns its pick result some months back and it seems I never
changed the orbit mode of the camera editor. I usually use the right
mouse button to rotate the camera, so this went unnoticed.
Fixed.
Thanks for the report!

Cheers,
Martin

2012/7/1 Peter C. <th3f...@gmail.com>:

[Peter C.]

Alright, good to hear it's fixed. I've been trying to get those static analysis logs, however apparently my Windows boot got borked. I'm going to run a diagnostic or two, but if that doesn't fix it, I'm going to have to format that drive.

Cheers,
Peter

[Peter C.]

Just as a heads up, I think I fixed my Windows boot, and I just tried to compile libRocket on Visual Studio 10 64 bit. The release zip file on the main site will not build on VS10 64 bit. I am about to submit a pull request on GitHub to fix the build issue, along with the multiple compile fix for VC10. I should also note I found a lot of potential buffer overflows. I'm probably going to spend a day or two to fix that.

[Peter C.]

Alright, I've ran PVS Studio against dtEntity (without the libRocket, as libRocket won't build on win64 in it's current state, even on the Github repo master, but I submitted a pull request with a fix). Attached is the log from PVS Studio just on dtEntity (sans libRocket support). At least a few issues stem from using unsigned int rather than size_t. That can cause at least some memory issues, and can also reduce performance. I would like to ask what your stance is on changing the uint property from an unsigned int to a size_t. That should help with 64 bit compatibility. Also moving over unsigned ints such as those in loops will help as well. I am already toying with replacing a few of the unsigned ints, and I was wondering if you had any problem with my swapping the uint property as well and putting the changed code up for you.

Cheers,
Peter

[Peter C.]

I've encountered an issue in the editor after recent commits. Apparently using mouse input freezes the particle component.

[Martin Scheffler]

Thank you for the analysis, gonna read it now. About the editor freeze:
can you attach the command line option --singleThread and see if
things still freeze?
Cheers,
Martin

2012/7/2 Peter C. <th3f...@gmail.com>:

[Peter C.]

Yup, that fixed the freezing in the editor (note I did not update to the newest revision/recompile and it fixed it). Testing with the latest revision,I got a build error in TextLabelComponent.h, attached is a fixed version of it. I also noticed a lot of new errors popping up including some potential overflows, I have attached a few fixed files and a new log based on a scan with the TextLabelComponent compile fix, along with a second log with the results of the changes I've done to silence some PVS Studio warnings.

Cheers,
Peter

[Peter C.]

Correction, --singleThread fixed it on moving the view on the scene, however clicking the property editor window freezes the particles again.

Cheers,
Peter

[Martin Scheffler]

Hi Peter,
thanks for the fixes, they are committed.
About the freeze: I can't really say anything about that because I
don't have a win 64 development environment yet. I know that
everything works fine on linux 64 though.
Can you see where the app freezes? Maybe you can start from the IDE
and press the pause button once everything is frozen, then you could
check out the stack trace

Cheers,
Martin

2012/7/2 Peter C. <th3f...@gmail.com>:

[Riccardo Corsi]

Hi guys,

about the size_t issue.
I agree that size_t is preferable when referring to memory size/container indexing as it guarantees portability.
But an unsigned int in general can be used for other purposes besides memory allocation,
so rather than replacing the uint property  with a size_t one, I'd rather add the size_t besides the uint, and use it only where appropriate.

How does this sound?

Ricky

[Martin Scheffler]

Hmm, isnt unsigned int implicitly casted to size-t? So usage should be no different

[Riccardo Corsi]

Hi Martin,

it's implicitly cast in comparisons, but a size_t type might be bigger (or even smaller I think) than an uint, depending on the platform.
It represents the maximum size that can be addressed on the target platform.

This thread explains quite well: http://stackoverflow.com/questions/131803/unsigned-int-vs-size-t
and gives a pointer to this article: http://eetimes.com/discussion/programming-pointers/4026076/Why-size-t-matters

Ricky

[Peter C.]

I think you may be right Ricky, I've noticed crashes with memory allocation errors when trying to add an enet component to a boid in the boids scene yesterday. I ran some traces and one of the values (not sure which one yet) is larger than it's capacity allows. Also, I figured I'd point this out, there is a new murmur hash implementation at https://code.google.com/p/smhasher/ that has a 32 bit hash with improved portability. I think the malloc issues were coming from the hash, however I'm not sure.

[Peter C.]

Alright, I tracked down the cause of the crash in the ENet component. It appears to crash when there are no dead reckoning components in the scene. Also creating a receiver without a sender for dead reckoning also appears to cause a crash. As for the freeze, I was unable to track down the freeze in Linux, however it happens in Windows, I'll have to track it down more.

[Martin Scheffler]

Hi Peter,
is it a crash or an assertion?
Can you send me a stack trace?
The whole enet system is not really finished yet, so bugs and missing
features are to be expected.

Cheers,
Martin

2012/7/4 Peter C. <th3f...@gmail.com>:

[Peter C.]

It was an assertion, the trace is:

/dev/dtentity-read-only/source/dtEntityNet/enetcomponent.cpp:94: virtual void dtEntityNet::ENetSystem::OnAddedToEntityManager(dtEntity::EntityManager&): Assertion `deadRecReceiverSystemInEntityManager' failed.
Aborted (core dumped)

[Peter C.]

I've run PVS Studio against the most recent change sets, this time including libRocket. Note that because of a build breaker that was only just fixed in libRocket's Git for windows 64 bit, it will require the latest libRocket Git, which requires the following patch to dtEntityRocket.

Attached files are the PVS Studio log, and the fixed code to support the libRocket git master. The git master is required for building on Windows 64 bit, therefore I recommend that this fix be put in dtEntity proper. It will break compatibility with libRocket stable, however it allows building on Windows 64 bit.

Also attached is a modified find extdeps cmake file that will make it a little easier to deal with building on Windows. It adds a search path of a folder called extdep in the source directory, and it looks for osg.lib instead of anything OpenAL related to deal with the possibility of a different OpenAL library name (a problem I came across on my end using the OpenAL SDK from Creative).

The libRocket Git is located at: https://github.com/lloydw/libRocket

My next objective is checking building with CEGUI, I'm in the process of getting that ready right now.

[Peter C.]

[Peter C.]

Sorry about the double post, I had server communication errors on my end according to a pop up message. I managed to find something interesting regarding the crashes on the osg forums, apparently someone else was having similar issues, and it may have been fixed in the OSG trunk, I'm going to try to set OSG to compile from trunk and test against that on my end, however I may go silent over the next week or so as I'll be focused on other things.
The thread: http://forum.openscenegraph.org/viewtopic.php?t=10533

Cheers,
Peter

[Martin Scheffler]

Hi Peter,

I checked in the cmake change.

About LibRocket: Maybe there is a way to make libRocket compile with
both trunk and release? Is there a version #define somewhere that we
can use? I'm quite OK with the current release version, why do you use
trunk?

About CEGui: I recently threw CEGui out of dtEntity core into a
plugin. I did not yet restore script bindings to CEGui. Will you use
CEGui from JavaScript or from C++?

2012/7/8 Peter C. <th3f...@gmail.com>:

[Peter C.]

The reason i use the git master is that the release breaks on compile on windows 64, it assumes win 32 and 64 use the same assembly in a math function, which isn't the case (32 and 64 bit have different assembly languages, so because it only assumes 32 bit, 64 bit will not compile. I recently sent them a pull request, which was accepted.) A flag should be possible, but i'm going to be getting some career training this week, so i don't have time to do it this week.

Regarding CEGUI, my main concern is just checking it while my license is still valid. I'm mainly concerned with getting an osgEarth.map file to load at the moment, which is not exactly playing nice. I'm going to select my gui toolkit later, but I'm leaning to libRocket.

Cheers,
Peter
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[Peter C.]

Attached are fixes for LibRocket on Windows 64, with a CMake flag to set them as optional.

[Peter C.]

A small fix for the previous files, I am an idiot and used #if instead of #ifndef. This one builds on Windows 64 using the LibRocket Git.

[Martin Scheffler]

Hi Peter,
I totally forgot about your fix, sorry!
Just checked it in, can you please test?
Thanks!
Martin

2012/8/7 Peter C. <th3f...@gmail.com>:

[Peter C.]

The commit breaks because the CMakeLists.txt is apparently overriding the option. Attached is a fixed CMakeLists.txt that builds.

Cheers,
Peter

[Martin Scheffler]

ok, checked in. Is the cmake option automatically converted to a
preprocessor macro?

2012/8/7 Peter C. <th3f...@gmail.com>:

[Martin Scheffler]

I just tested it and it seems the changes come to play if the cmake
option is NOT set. I just fixed the code to now use the changed
librocket api when the option is set.
Does everything work now?

2012/8/8 Martin Scheffler <martins...@gmail.com>:

[Peter C.]

Yup, it works now.