[Solved] Crash in TextBox::setText (0.9.6b)

mikachu

01-09-2007 13:43:02

Hello,
I've set up quickGUI for code::blocks+mingw, and everything works, except the setText function in QuickGUITextBox.cpp crashes (and so the demo crashes at init time).
The crash itself occurs at line 178, my debugger says:
else
{
Quad* character = mText->getCharacter(mCursorIndex - mVisibleStart);
Point pos = character->getPosition();
Size size = character->getSize();
cursorPos.x = pos.x;
cursorPos.y = pos.y + (size.height / 2);
}


Here, the 'character' is NULL, so the call simply crashes.

Is this problem already solved in SVN?

mikachu

01-09-2007 17:00:42

Ok, I've modified the above code to include a condition " if character != NULL ", so the demo doesn't crash anymore, BUT no text is displayed anywhere (contrary to the demo screenshot)

kungfoomasta

01-09-2007 19:19:21

That code is in the function TextBox::_positionCursor(). I've been tempted to add that safegaurd in there, but the problem is, if you make it into that else statement, there should be at least 1 character. I don't really have enough information to reproduce/investigate this from what you've given me. The demo runs fine on my machine using Visual Studio 2005, and unless you changed the demo code, the behavior should be similar.

When the call to TextBox::setText is made, is this the very first call to that function? What is the string that is passed in? What is the value of "cursorIndex" when you enter TextBox::setCursorIndex? Inside setCursorIndex, a call is made to TextBox::_determineVisibleTextBounds. After this call, what is the value of mVisibleStart and mVisibleEnd? If I had to guess, I would say _determineVisibleTextBounds is not correctly setting the visible start and end of the text.

Basically the TextBox has a caption, and the Text object displays a substring of that caption, unless the caption can fit within text bounds. The caption is being set, which is while you enter that else, but the text has not rendered any characters, which is why NULL is returned when asked for the first character. If the caption is not "", there should definately be some rendering of characters from the Text object.

If you could step into _determineVisibleTextBounds and tell me the cursorIndex, as well as the current mVisibleStart/End when entering the function, and the resulting mVisibleStart/End after exiting the function, we can find out more about what is happening.

Sorry I can't help more. I'm interested to figure this out, if you work with me. :)

mikachu

01-09-2007 22:49:15

I've restarted from the 0.9.6b sources (to keep my modifications away) and rebuilt everything.
The first crash I got was not the one I described, maybe I had already solved it. It also occurs in the demo, during the call to guimanager::setup.

This is the call stack :
guimanager::injectmousemove (429)
guimanager::setactivesheet(560)
guimanager::setup(626)
demo createscene code

In fact, when I check it, hitWidget is null, thus causing the crash.


// Now get the widget the cursor is over.
Widget* hitWidget;
Widget* wItr = mActiveWidget;
while( (wItr != NULL) && ((hitWidget = wItr->getTargetWidget(args.position)) == NULL) )
wItr = wItr->getParentWidget();

// NOTE: Widget is only detected if it is enabled.
args.widget = hitWidget;

if( (mWidgetContainingMouse->getInstanceName() != hitWidget->getInstanceName()) )
{
mWidgetContainingMouse->onMouseLeaves(args);
mWidgetContainingMouse = hitWidget;
mWidgetContainingMouse->onMouseEnters(args);
}


but that one is an easy one, I just replaced if ( (mWidgetContainingMouse->getInstanceName() != hitWidget->getInstanceName()) ) by if(hitWidget != NULL && (mWidgetContainingMouse->getInstanceName() != hitWidget->getInstanceName()) )


Now, back to my initial problem :

The context of the call of setText is the following (from the demo) :


usernameTB = mSheet->createTextBox(Rect(0.225,0.6,0.2,0.05));
usernameTB->setText("\tThe quick brown fox jumped over the red fence.");



When the call to TextBox::setText is made, is this the very first call to that function? What is the string that is passed in?

Yes. The string passed in is "\tThe quick brown fox jumped over the red fence."

What is the value of "cursorIndex" when you enter TextBox::setCursorIndex?

47

nside setCursorIndex, a call is made to TextBox::_determineVisibleTextBounds. After this call, what is the value of mVisibleStart and mVisibleEnd?

47 for both of them

If you could step into _determineVisibleTextBounds and tell me the cursorIndex, as well as the current mVisibleStart/End when entering the function, and the resulting mVisibleStart/End after exiting the function, we can find out more about what is happening.

before entering the function:
cursorindex=47
mvisiblestart=0
mvisibleend=0
after the function:
mvisiblestart=47
mvisibleend=47

If you want to reproduce the bug, I can provide you with my project files for code::blocks...

kungfoomasta

02-09-2007 02:56:11

I'm not familiar with code blocks, mingw and how to set everything up for use... :(

Regarding the NULL safe gaurd, that's another place where it shouldn't be needed. I removed that gaurd a few versions ago, because hitWidget should never be NULL. Even if you see a blank screen, there is a DefaultSheet widget that would be the hitWidget. So I'm not sure what's going on there..

Regarding the text issue, it doesn't make any sense to me. Here is a snippet from _determineVisibleTextBounds:


// Shift text right to show portion of caption ending at cursorIndex.
else if( cursorIndex > mVisibleEnd )
{
mVisibleEnd = cursorIndex;
mVisibleStart = mVisibleEnd - 1;
while( (mText->calculateStringLength(mCaption.substr(mVisibleStart,mVisibleEnd - mVisibleStart)) <= mTextBoundsAbsoluteDimensions.width)
&& (mVisibleStart > 0) )
--mVisibleStart;

// When we exit the while loop, we are either at visibleStart == 0, or we have exceeded the width limitation.
if( mText->calculateStringLength(mCaption.substr(mVisibleStart,mVisibleEnd - mVisibleStart)) > mTextBoundsAbsoluteDimensions.width )
++mVisibleStart;
}


If this held true before entering the function:

cursorindex=47
mvisiblestart=0
mvisibleend=0

Then we would definately enter this else branch. Even if the while loop and if inside the else were not entered, mVisibleEnd would be 47 and mVisibleStart would be 46. Are you able to step into this function and verify this?

mikachu

02-09-2007 13:33:22

Yes, this else branch is executed, and after the while, mVisibleStart=46, mVisibleEnd=47 and cursorIndex=47.

Note that mTextBoundsAbsoluteDimensions.width is around 0.2 (maybe there is a mismatch between relative and 'number of chars' coordinate??), so the next if condition is true, and therefore mVisibleStart is incremented, thus the result (mVisibleStart=47).

mikachu

02-09-2007 16:28:38


Regarding the NULL safe gaurd, that's another place where it shouldn't be needed. I removed that gaurd a few versions ago, because hitWidget should never be NULL. Even if you see a blank screen, there is a DefaultSheet widget that would be the hitWidget. So I'm not sure what's going on there..

Actually, there IS a DefaultSheet, but widget::gettargetwidget() returns NULL because isPointWithinBounds() returns false because mPixelDimensions.width = 0 and mPixelDimensions.height= 0...
DefaultSheet is created with createSheet, with empty texture name. In the constructor, absolute dimension is inited correctly, but not pixeldimensions, because the getRenderWindowHeight and getRenderWindowWidth return 0.
For the moment I have no idea why window width and height are wrong, because it is correctly initialised...

kungfoomasta

02-09-2007 19:31:40

So you are calling GUIManager::setup, with the correct window dimensions? The Sheet widget automatically creates a widget of relative dimensions (0,0,1,1), to take up the whole screen.

See if you can step inside the Sheet constructor down to the Widget constructor, and see what the values are for the window dimensions:


Rect Widget::absoluteToPixelDimensions(const Rect& dimensions)
{
Ogre::Real renderWindowWidth = static_cast<Ogre::Real>(mGUIManager->getRenderWindowWidth());
Ogre::Real renderWindowHeight = static_cast<Ogre::Real>(mGUIManager->getRenderWindowHeight());

return Rect(
static_cast<int>(dimensions.x * renderWindowWidth),
static_cast<int>(dimensions.y * renderWindowHeight),
static_cast<int>(dimensions.width * renderWindowWidth),
static_cast<int>(dimensions.height * renderWindowHeight)
);
}


I should probably remove my functions I have that derive the relative/absolute/pixel coordinates, and imitate the MouseCursor class, which just calls setSize and setPosition, since they should be maintaining the 3 dimensions anyway.

mikachu

03-09-2007 10:00:25

Yes, I call setup with the good dimensions.
The problem seems to be caused by compiler weirdness on the singleton, because I can trace 2 calls to GUIManager constructor (so the second call gives another instance, with window dimensions to 0,0, even if the dimensions were ok for the first created instance).
I have to investigate this a little bit more...
If someone here is experimented with MinGW, I'd like to know his point of view about this..

mikachu

03-09-2007 23:07:09

I tried something, and I saw that all the bugs I described are linked to the same cause. By adding a quick hack to the GUIManager constructor, everything works well, and I can see text, etc...

This is the hack :

GUIManager::GUIManager() :
mMouseCursor(0),
mActiveSheet(0),
mWidgetContainingMouse(0),
mActiveWidget(0),
mClickTimeout(75),
mAutoNameSheetCounter(0),
mQueueID(Ogre::RENDER_QUEUE_OVERLAY)
{
mRenderWindowWidth=1024;
mRenderWindowHeight=768;
}

Without this hack, there is a bug, because each time GUIManager::getSingletonPtr() is called, a new instance of GUIManager is created, with window size of (0,0).

But I still don't understand why it behaves like this. Maybe the configuration of ".h" and ".cpp"? If I do the same singleton implementation as yours in an empty project, there is no bug like that.

So, I tried something else: I just put the code for getsingleton() and getsingletonptr() in "quickguimanager.cpp", and declaration in "quickguimanager.h", it also clears all bug and is far less hacky.

End word: maybe Visual Studio compiler had some clever stuff to detect this kind of problem and automatically solve it?

kungfoomasta

04-09-2007 05:36:29

I've made a few changes regarding the Singleton's.

under private:
static GUIManager mGUIManager;

And in QuickGUIManager.cpp:

namespace QuickGUI
{
GUIManager GUIManager::mGUIManager;

GUIManager::GUIManager() :
mMouseCursor(0),
mActiveSheet(0),
mWidgetContainingMouse(0),
mActiveWidget(0),
mClickTimeout(75),
mAutoNameSheetCounter(0),
mQueueID(Ogre::RENDER_QUEUE_OVERLAY)
{
}

GUIManager::~GUIManager()
{
}

GUIManager& GUIManager::getSingleton()
{
return mGUIManager;
}

GUIManager* GUIManager::getSingletonPtr()
{
return &mGUIManager;
}


QuickGUIManager.h:

public:
static GUIManager& getSingleton();

static GUIManager* getSingletonPtr();


I also applied this to the MouseCursor, and committed it to SVN. Let me know if this doesn't fix the problems you are seeing. Also, zini is experiencing some issues under Code Blocks, maybe you can send him you project config? Or maybe we can put it in SVN somewhere? :)

Zini

04-09-2007 08:46:33

Don't bother with the Code::Blocks project file. I have almost sorted it out (but if mikachu has a fully working project file, putting it into SVN would certainly a good idea).

I am having some problems with this implementation from the software-engineering/C++ language point of view. Sounds like you are running into the »static initialization order fiasco«: http://www.parashift.com/c++-faq-lite/c ... #faq-10.12
After all non-trivial static members are almost as evil as global variables.

After reviewing your code a bit more, I found that the constructor and the destructor of the GUIManager are empty, which they certainly shouldn't be, since GUIManager is a highly non-trivial class. As far as I can see you have at least some resources leaks in the destructor (sheet?). And once you have fixed them, you will certainly run into the problem mentioned above (though it is called initialization order fiasco, the same applies to destruction of static objects).

If I understand your code right, the setup method is some kind of replacement constructor. That means you can't use any part of the GUIManager, before this method is called (correct me, if I am wrong; I hadn't the time to review the whole class). But that is the role of the constructor and it really shouldn't be moved to another method.
You probably created the setup method, because singleton constructor's can't take arguments, but this only indicates that the singleton pattern isn't really applicable here (IMHO a highly overrated design pattern anyway).

May I propose a different approach?


class GUIManager
{
public:

GUIManager (some arguments ...);

~GUIManager();

static GUIManager& getInstance();

private:

static GUIManager *mInstance;
};

// implementation

GUIManager::GUIManager (some arguments ...)
{
if (mInstance)
throw std::logic_error ("only one instance of GUIManager allowed");

// some stuff

mInstance = this;
}

GUIManager::~GUIManager()
{
mInstance = 0;

// some stuff
}

GUIManager& GUIManager::getInstance()
{
if (!mInstance)
throw std::logic_error ("no instance of GUIManager");

return *mInstance;
}

GUIManager *GUIManager::mInstance = 0;


(untested example code)

With this pattern you can construct/destruct the GUIManager like any other object, but you still have singleton-like access.

mikachu

04-09-2007 09:40:02

@ kungfoomasta :
I tested your modifications before coming to the office, and it doesn't even compile (!). I have already had this kind of weirdness when I tried to add a static variable to GUIManager, there was something wrong during the linking phase... I'll try to sort it out this evening...

Zini

04-09-2007 10:00:52

Looks like I am not the only one, who has problems with the imported VC project ;)

kungfoomasta

04-09-2007 16:22:35

Ah, Code Blocks is so picky. :P

Zini, your approach sounds fine. IMO setup/shutdown adds another level of flexibility. While it adds the step of making sure the user calls setup before use, it is possible to shutdown the GUIManager and then set it up again, which can be used in scenarios like changing resolution during execution. Everything has to be shutdown including Ogre, and then re-created. (Primary render window with new resolution, hooking in OIS, etc.)

I think I will use your approach, however the constructor will call setup. :P This way the user won't have to take this first step initially, so only advanced users need to worry about it.

I'm also considering looking into rendering via viewport, which means GUIManager won't be a singleton anymore. (one GUIManager per viewport) I believe this is what would need to happen to support multiple windows, for example.

When I get back from work today I will look into this more. I will start out by removing Singleton from GUIManager, and making the constructor/destructor properly manage the class. I can use your implementation for the MouseCursor, which will still remain a singleton. Sound good?

[edit] well if I can destroy and create guimanager's, setup/shutdown is not really needed.. [/edit]

Zini

04-09-2007 16:26:24

Fine with me.

Edit: btw. Code::Blocks is not picky at all. Only the VC-importer is making trouble. Projects created in Code::Blocks usually work fine.

mikachu

06-09-2007 22:16:32

Here are my QuickGUI project files for Code::Blocks + MingW :

In the root directory of QuickGUI :
http://rapidshare.com/files/53882268/QuickGUI.workspace.html

In the 'QuickGUI' directory:
http://rapidshare.com/files/53882269/QuickGUI.cbp.html

In the 'QuickGUIDemo' directory:
http://rapidshare.com/files/53882270/QuickGUIDemo.cbp.html

I'd like to test when the modifications made when they are committed to SVN, I will post here to say if it correctly compiles / doesn't crash when compiled with code::blocks..

kungfoomasta

06-09-2007 22:57:03

Awesome, I will add these in tonight! Are Code::Blocks workspace files equivalent to .NET solution files, and cbp with vcproj? (I'm assuming they are, which means they would change if I add/remove/rename files within the projects.) Are these based off SVN or the 0.9.6b?

kungfoomasta

07-09-2007 06:15:34

I have committed the files to SVN. Thanks again for supplying these!

mikachu

07-09-2007 09:07:02

Awesome, I will add these in tonight! Are Code::Blocks workspace files equivalent to .NET solution files, and cbp with vcproj? (I'm assuming they are, which means they would change if I add/remove/rename files within the projects.) Are these based off SVN or the 0.9.6b?
Indeed, they are.
The project and workspace files I submitted are 0.9.6b based, I will get the source from SVN and upgrade them this weekend to add new source files.