Jump to content
The Dark Mod Forums

New Colour Scheme Handling


Recommended Posts

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.

Link to comment
Share on other sites

  • Replies 72
  • Created
  • Last Reply

Top Posters In This Topic

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...

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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!

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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. :)

Link to comment
Share on other sites

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".

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

 

Ahem.

 

On a related note, if we provisionally aimed for next weekend for a 0.7 release,
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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?

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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

    • Petike the Taffer  »  DeTeEff

      I've updated the articles for your FMs and your author category at the wiki. Your newer nickname (DeTeEff) now comes first, and the one in parentheses is your older nickname (Fieldmedic). Just to avoid confusing people who played your FMs years ago and remember your older nickname. I've added a wiki article for your latest FM, Who Watches the Watcher?, as part of my current updating efforts. Unless I overlooked something, you have five different FMs so far.
      · 0 replies
    • Petike the Taffer

      I've finally managed to log in to The Dark Mod Wiki. I'm back in the saddle and before the holidays start in full, I'll be adding a few new FM articles and doing other updates. Written in Stone is already done.
      · 4 replies
    • nbohr1more

      TDM 15th Anniversary Contest is now active! Please declare your participation: https://forums.thedarkmod.com/index.php?/topic/22413-the-dark-mod-15th-anniversary-contest-entry-thread/
       
      · 0 replies
    • JackFarmer

      @TheUnbeholden
      You cannot receive PMs. Could you please be so kind and check your mailbox if it is full (or maybe you switched off the function)?
      · 1 reply
    • OrbWeaver

      I like the new frob highlight but it would nice if it was less "flickery" while moving over objects (especially barred metal doors).
      · 4 replies
×
×
  • Create New...