Jump to content
The Dark Mod Forums

Recommended Posts

Posted
Thanks, I will try to fix this! (Strange, why doesn't it crash on Windows?)

 

Because it's Undefined Behaviour, which may crash, may not crash, or may reformat your harddrive and murder your kids (hence "undefined").

 

Concerning the gtk_main() loop: I only implemented this, because I got some really weird crashes when I don't call the loop. For example, the signal "changed" got called only once and crashed DR immediately when it got called the second time. Perhaps I did something wrong (most probably I did :)), but the problem magically disappeared after using gtk_main(), so I thought this call was vital for GTK dialogs to function correctly.

 

That will be because otherwise the ColourSchemeEditor instance doesn't last long enough to process any signals - it is a local object that is created in EditColourScheme() and immediately deleted, after which control is passed back to the main gtk_main() loop. As soon as you click on a button, a callback function will be called with a this pointer which is no longer valid, hence more Undefined Behaviour.

 

Heh, I've just realised the way I avoided this for the TextureChooser (in the EntityInspector) was to allocate it with new in the calling function and then call "delete self" in the GTK callback for destroy. Not quite a "delete this" but it's close.

 

This actually means that there is an easy fix for your double free, change EditColourScheme to this:

 

void EditColourScheme() {
new ui::ColourSchemeEditor(); // self-destructs in GTK callback
}

 

Note the comment to indicate that this is not a memory leak, which is what it looks like.

  • Replies 72
  • Created
  • Last Reply

Top Posters In This Topic

Posted
Heh, I've just realised the way I avoided this for the TextureChooser (in the EntityInspector) was to allocate it with new in the calling function and then call "delete self" in the GTK callback for destroy. Not quite a "delete this" but it's close.

Ah, I see. Another lesson learned...

Posted
Ah, I see. Another lesson learned...

 

Indeed. If you have not already seen the C++ FAQ, I suggest looking through it, there is a lot of important/useful information in there.

 

http://www.parashift.com/c++-faq-lite/

 

The difference between stack and heap-allocated objects is one of those things which always bites new C++ programmers.

Posted

Thanks for the FAQ, I have a couple of purple, visited links on that site :). I know the difference between stack and heap, but obviously I just didn't think about this.

 

edit: The changes are now on SVN, I hope I got it right this time. Thanks for your support, Orbweaver!

Posted

Just to be sure, what version of Glew should I be using? Seems to be hanging on a section where it mentions Glew.

 

So weird. No errors...nothing.

 

This is what I'm getting.

 

/ToolbarCreator.o build/debug/radiant/ui/common/TexturePreviewCombo.o build/debug/radiant/ui/mediabrowser/MediaBrowser.o build/debug/radiant/ui/menu/FiltersMenu.o build/debug/radiant/filters/XMLFilter.o build/debug/radiant/ui/common/ColourSchemeEditor.o -Lbuild/debug/libs -Llibs -lmathlib -lmath -lcmdlib -lprofile -lgtkutil -lexception -lxmlutil -lcolourscheme -lboost_regex -lgtkglext-x11-1.0 -lgdkglext-x11-1.0 -lGLU -lGL -lGLEW

 

Edit:

 

Hmm, looks like it might just be taking longer than usual to compile for some reason. It has moved on. I'll shut up now.

Posted

Just a couple of things (I know the feature is not finished yet):

 

1. I am not sure that a separate static library is necessary for this. I would put all of the colour scheme handling code in a separate subdirectory under radiant/, and allow modules to look up colours via a single method exposed on the GlobalRadiant() interface -- e.g. Vector3 getColourFor(const std::string& name). This means that modules wouldn't need to process ColourScheme objects themselves, and hence would not need to be linked with an additional library (which will tend to slow things down).

 

2. Are you sure the ColourItem data object isn't just a tad complex? It's storing two versions of the colour (as a Vector3 and a GdkColor) along with the name. The name shouldn't be required if these objects are stored in a map which is already indexed by name (the ColourItemMap), and a Vector3 can be easily translated to a GdkColor in a cast method, without needing to store both. I would probably either define the ColourItemMap as std::map<std::string, Vector3>, or create a subclass of Vector3 (e.g. Colour3, although I think that is already used) which inherits all of Vector3's methods but provides an additional operator cast to GdkColor*. Note also that Vector3 already has methods to convert to and from a string, so you don't need to manually use stringstreams to do this.

Posted
1. I am not sure that a separate static library is necessary for this. I would put all of the colour scheme handling code in a separate subdirectory under radiant/, and allow modules to look up colours via a single method exposed on the GlobalRadiant() interface -- e.g. Vector3 getColourFor(const std::string& name). This means that modules wouldn't need to process ColourScheme objects themselves, and hence would not need to be linked with an additional library (which will tend to slow things down).

Ok, will change this.

 

2. Are you sure the ColourItem data object isn't just a tad complex? It's storing two versions of the colour (as a Vector3 and a GdkColor) along with the name. The name shouldn't be required if these objects are stored in a map which is already indexed by name (the ColourItemMap), and a Vector3 can be easily translated to a GdkColor in a cast method, without needing to store both. I would probably either define the ColourItemMap as std::map<:string vector3>, or create a subclass of Vector3 (e.g. Colour3, although I think that is already used) which inherits all of Vector3's methods but provides an additional operator cast to GdkColor*. Note also that Vector3 already has methods to convert to and from a string, so you don't need to manually use stringstreams to do this.

In my first version I stored the ColourItems in a vector and I recently replaced this by a std::map, therefore the redundancy. I will fix this.

 

I must admit that I implemented the ColourItem class because I wanted to get some practice with classes. But as I don't want to clutter the codebase with redundant functions, I will of course remove it. One thing that I somehow like about the ColourItem class is the constructor that takes an xml::node to retrieve the colour information, but I guess this can also be done another way. I will stick to the Colour3 inheritance you suggested.

 

I guess my code can be classified under "bad design", so I'm sorry for that. Four weeks of C++ are not that much I reckon, but I will work on that. :)

Posted
I guess my code can be classified under "bad design", so I'm sorry for that. Four weeks of C++ are not that much I reckon, but I will work on that. :)

 

Hey, don't knock yourself down. :) I think you're doing a great job. What better way to learn than by doing?

Posted

Ok, I'm running into the first problem:

 

I have this operator in my ColourItem class:

ColourItem::operator GdkColor* () {
return &_gdkColor;
}

which returns a pointer to the internal GdkColor member variable. I use this for, say, the gtk_color_button_new_with_color() method, which expects a pointer to a GdkColor variable. How would I do this with no internally stored GdkColor variable?

 

If I did it this way:

ColourItem::operator GdkColor* () {
	GdkColor returnValue;
	// ... pump the rgb values into the returnValue
	return &returnValue;
}

I would return a pointer to a variable that would get destroyed immediately after return. How should I do this in a clean way? For GTK+ the GdkColor object has to be stored somewhere for the ColourButtons to function correctly, hasn't it? I think this was my main reason for making the _gdkColor representation an actual member variable of the ColourItem class, so that I can easily provide a pointer to an object that stays "alive".

Posted
I guess my code can be classified under "bad design", so I'm sorry for that. Four weeks of C++ are not that much I reckon, but I will work on that. :)

 

Nothing wrong with your designs, it is often the case that somebody else can point out an issue with the first incarnation that might merit a different approach -- my own designs are far from perfect, which is why I have spent the past few days ripping out and replacing much of the Entity Inspector code.

 

I would return a pointer to a variable that would get destroyed immediately after return. How should I do this in a clean way? For GTK+ the GdkColor object has to be stored somewhere for the ColourButtons to function correctly, hasn't it? I think this was my main reason for making the _gdkColor representation an actual member variable of the ColourItem class, so that I can easily provide a pointer to an object that stays "alive".

 

A few ideas:

  1. Keep the GdkColor* internally, this could be justification for maintaining it, although you would have be careful to ensure that the two internal representations were consistent.
  2. Have an operator GdkColor() that returns an actual struct, rather than a pointer. You might then have to use an explicit cast, such as
    gtk_color_button_new_with_color(&(static_cast<GdkColor>(colorObject)));


    which you might feel is a bit ugly, but should work -- alternatively, use a temporary to make it obvious what is going on

    GdkColor color(colorObject);
    gtk_color_button_new_with_color(&color);


     

  3. If the only reason you need a GdkColor* at all is to create a GtkColorButton, you could just have a method on the ColourItem which creates and returns the GtkColorButton* directly.

Posted

Thanks, I will try to change the class so that it is as simple as possible without having to change the entire ColourSchemeEditor methods - I will particularly focus on removing the internal GdkColor storage.

 

On a different note: has someone managed to change the colour of a selected brush (both XY and camera view)? Looking at the code, I suspect that these options were never working.

 

I also noticed that "Grid Minor Small" and "Grid Major Small" aren't used anywhere in the code, so I'll kick them both.

Posted
On a different note: has someone managed to change the colour of a selected brush (both XY and camera view)? Looking at the code, I suspect that these options were never working.

I also noticed that "Grid Minor Small" and "Grid Major Small" aren't used anywhere in the code, so I'll kick them both.

 

It wouldn't surprise me if there is a whole load of unused stuff in there. I can't actually find the entity lime green anywhere in the scheme editor, so maybe that particular colour is hardcoded somewhere.

 

On a related note, if we provisionally aimed for next weekend for a 0.7 release, do you think the Scheme Editor would be ready? There is no problem if it isn't (and I certainly don't want you to rush things), but I wouldn't want to put out a release if you were only a couple of days away from finishing the feature.

Posted

I have one colour left to implement before it's finished, so the feature should definitely make it till the weekend. I already removed the unused stuff and will try to add some more options to the scheme editor as I stumble on them.

Posted

Ok, I'm done with the colours. Should I include the three other schemes ("Maya Emulation", "Q3Radiant Original", "Black and green") in the install/user.xml or aren't they used by anyone anyway?

 

I will do an SVN update and fresh compile and then it should be ready to commit.

 

edit: committed.

Posted

The original themes are now on SVN.

 

There is not much difference between the default scheme and the QE3Radiant one (that only makes the minor grid invisible). All themes except the default one have the readonly flag unset, i.e. they can be deleted by the user.

Posted
Damn, not to be pushy... When would an unofficial sneak preview be available? ^_^ From all the talk lately and NH's teasing, it sounds like there's a lot new to see.

 

:) I know how you feel. I installed linux for the very reason of being able to test things out as they're added. While it is possible to build on windows, I just found it too much of a hassle to get it working. You might have to get Ubuntu running on your system too. ;)

 

I think orb was talking about having 0.7 out this coming weekend and Spar is going to make a small update package for the beta mappers. :) We'll be releasing them at the same time.

Posted

I guess it was more of an "in general" than directly about this next release. A system where the internal team gets frequent builds, for the sake of feedback, testing, and.. well, fun, far more often than the official drop releases? That's what I'm asking.

Posted
I guess it was more of an "in general" than directly about this next release. A system where the internal team gets frequent builds, for the sake of feedback, testing, and.. well, fun, far more often than the official drop releases? That's what I'm asking.

 

Oh I see. Well, there is always the compile-it-yourself option which New Horizon has taken up (it's not actually as hard as it sounds), plus we can do more frequent point releases if people want it. I have been a bit slack with the releases lately for the simple reason that the most enthusiastic DR users were already compiling from source.

Posted
Oh I see. Well, there is always the compile-it-yourself option which New Horizon has taken up (it's not actually as hard as it sounds), plus we can do more frequent point releases if people want it. I have been a bit slack with the releases lately for the simple reason that the most enthusiastic DR users were already compiling from source.

 

Only drawback I have is that I'm compiling under Linux, so I can't test it under windows. I might take another stab at compiling under windows, it should be a lot easier the second time around. I want to be able to bounce back and forth to check for platform specific issues anyway.

 

All the same, I would definitely support more frequent point releases since the majority of our mappers are likely going to be using windows anyway.

Posted

I just checked out the latest changes. It's looking very intuitive, I particularly liked how the changes are immediately visible so you can see what each scheme looks like in the editor window.

 

There does seem to be a minor bug, in that if you edit a particular colour in a scheme, then click Cancel, the colour change is not reverted. Could the original colours be saved before an edit so that they can be reverted if the user chooses Cancel rather than OK?

Posted
There does seem to be a minor bug, in that if you edit a particular colour in a scheme, then click Cancel, the colour change is not reverted. Could the original colours be saved before an edit so that they can be reverted if the user chooses Cancel rather than OK?

Hm, it really should be reverted, as I reload all the colour info from the XMLRegistry on cancel. The colourbutton callbacks just alter the in-memory colour schemes, not the registry itself. Only upon pressing OK the changes are written into the registry. I will try to reproduce this.

 

edit: yep, is reproducible.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


  • Recent Status Updates

    • Frost_Salamander

      @taaaki Wiki seems broken. Main page works but click any link and:

      · 2 replies
    • Goblin of Akenash

      Another goblin secrets episode on the way with the new update
      Focuses are:
      supporting 100% runs on shadow playstyles
      fixing the softlock
      and a tiny visual upgrade

      · 0 replies
    • STiFU

      Sorry for not being around enough lately. I am extremely busy at the moment. We are trying to build a home and the German bureaucracy is killing me! Meanwhile, child number 2 is underway. 🙂
      · 0 replies
    • cvlw

      Yo TDMers.
      It has been a while.  This past year or so I have encountered some tough family issues, became a grandfather, had a job change, and then Steam offered Dishonored and Dishonored 2 along with all DLC for like 5 dollars.  So... have played that.  What a game; highly recommend.
      I am looking to resume TDM activity soon.
      Clint
      · 2 replies
    • Airship-Ballet

      If anyone would like some ambient sounds for any of their work please do hit me up - I've tons of strings both physical and sampled that I love making loops with
      · 2 replies
×
×
  • Create New...