IOAttributes implementation

[chronologicaldot]

I was rummaging through the IOAttributes-related code to add a method for serializing ONLY the desired attributes of a GUI element when I noticed io::SAttributeReadWriteOptions can use the flags of E_ATTRIBUTE_READ_WRITE_FLAGS. One of those flags is documented as being "Serialization/Deserializion is done for an editor property box" (namely EARWF_FOR_EDITOR). Sadly, these flags are never used in any of the implementations.

I assume the purpose for this flag is to return ONLY the unique values of an object. For example, for a GUI text box, only the text is returned. For a GUI checkbox, only the "is checked" status is returned (um... "set").
Since that's what I need, I intend to implement this for a number of GUI elements. (I'm not sure about scene elements.) But I didn't know if that was the original intention.

Anyone know if these flags are used anywhere in the engine?

NOTE: io::SAttributeReadWriteOptions is used in the interface in IAttributeExchangingObject, from file IAttributeExchangingObject.h

[CuteAlien]

On a quick search it seems to be used in CSceneNodeAnimatorFollowSpline, CSceneNodeAnimatorTexture annd in CParticleSystemSceneNode. And if I had to guess then it was likely something Niko once needed for irrEdit.

[chronologicaldot]

I decided to simply create a couple new serialization options for the purpose of reducing the amount of information returned to a scripting language, which speeds up information-access and eliminates the need to retain all of the info about the GUI element in variables in the scripting lang. (Otherwise, you would have to use that info again and again every time you wanted to change a single attribute via deserializeAttributes().)

I've created some diff files. The following are original diff files. Further down, I'll post some that are not original diffs since I had to edit out some unrelated stuff. I can post full versions of those if you prefer, but I wanted to get the point across.

Original Diffs

IAttributeExchangingObject.h

Code: Select all

 
29c29,37
<   EARWF_USE_RELATIVE_PATHS = 0x00000004
---
>   EARWF_USE_RELATIVE_PATHS = 0x00000004,
> 
>   //! Serialization/Deserialization is done for a scripting language extracting only a few necessary values
>   EARWF_FOR_SCRIPTING_LANG = 0x00000008,
> 
>   //! Serialization/Deserialization is done for a scripting language, extracting all values specific to something
>   //! as opposed to generic values also belonging to parent classes.
>   //! For example, this enables serialization of all IGUIButton attributes but no IGUIElement (the parent) attributes.
>   EARWF_FOR_SCRIPTING_LANG_EXTRA = 0x00000010
65a74,99
> 
> //! Indicates the read/write option is for files
> inline bool isReadWriteForFile(const io::SAttributeReadWriteOptions* options) {
>   if ( options )
>       return (options->Flags & EARWF_FOR_FILE) > 0;
>   return false;
> }
> 
> //! Indicates the read/write option is for editors
> inline bool isReadWriteForEditor(const io::SAttributeReadWriteOptions* options) {
>   if ( options )
>       return (options->Flags & EARWF_FOR_EDITOR) > 0;
>   return false;
> }
> 
> inline bool isReadWriteForScriptingLang(const io::SAttributeReadWriteOptions* options) {
>   if ( options )
>       return (options->Flags & EARWF_FOR_SCRIPTING_LANG) > 0;
>   return false;
> }
> 
> inline bool isReadWriteForScriptingLangExtra(const io::SAttributeReadWriteOptions* options) {
>   if ( options )
>       return (options->Flags & EARWF_FOR_SCRIPTING_LANG_EXTRA) > 0;
>   return false;
> }
 

IGUIElement

Code: Select all

 
773a774,776
>       if ( io::isReadWriteForScriptingLang(options) || io::isReadWriteForScriptingLangExtra(options) )
>           return;
> 
789a793
>       out->addString("ToolTip", getToolTipText().c_str());
797a802,804
>       if ( io::isReadWriteForScriptingLang(options) || io::isReadWriteForScriptingLangExtra(options) )
>           return;
> 
820a828
>       setToolTipText(in->getAttributeAsStringW("ToolTip").c_str());
 

CGUIButton.cpp

Code: Select all

 
552,553d551
<   IGUIButton::serializeAttributes(out,options);
< 
557a556,560
>   IGUIButton::serializeAttributes(out,options);
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>       return;
> 
602d604
<   IGUIButton::deserializeAttributes(in,options);
605a608,612
> 
>   IGUIButton::deserializeAttributes(in,options);
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>       return;
 

CGUICheckBox.cpp

Code: Select all

 
231c231,238
<   IGUICheckBox::serializeAttributes(out,options);
---
>   // Options disabled for editor
>   if ( ! io::isReadWriteForScriptingLang(options) )
>   {
>       IGUICheckBox::serializeAttributes(out,options);
>       out->addBool("Border",  Border);
>       out->addBool("Background", Background);
> 
>   }
234,235d240
<   out->addBool("Border",  Border);
<   out->addBool("Background", Background);
241a247
>   // Options enabled for editor
242a249,253
> 
>   if ( io::isReadWriteForScriptingLang(options) ) {
>       return;
>   }
> 
 

CGUIEditBox.cpp

Code: Select all

 
1634a1635,1639
>   if ( io::isReadWriteForScriptingLang(options) ) {
>       out->addString("Caption", Text.c_str());
>       return;
>   }
> 
1659a1665,1669
>   if ( io::isReadWriteForScriptingLang(options) ) {
>       setText(in->getAttributeAsStringW("Caption").c_str());
>       return;
>   }
> 
 

CGUIImage.cpp

Code: Select all

 
182a183,186
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>       return;
> 
200a205,208
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>       return;
> 
 

CGUISpinBox.cpp

Code: Select all

 
322a323,328
> 
>   if ( io::isReadWriteForScriptingLang(options) ) {
>       out->addFloat("Value", getValue());
>       return;
>   }
> 
334a341,346
> 
>   if ( io::isReadWriteForScriptingLang(options) ) {
>       setValue(in->getAttributeAsFloat("Value"));
>       return;
>   }
> 
 

CGUIStaticText.cpp

Code: Select all

 
589a590,595
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       out->addString("Caption", Text.c_str());
>       return;
>   }
> 
609a616,621
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       setText(in->getAttributeAsStringW("Caption").c_str());
>       return;
>   }
 

[chronologicaldot]

Shortened Diffs
These are shortened just to get the point across of what the changes would be:

CGUIComboBox.cpp

Code: Select all

 
492c486,489
<   IGUIComboBox::serializeAttributes(out,options);
---
>   // Options disabled for scripting lang
>   if ( !io::isReadWriteForScriptingLang(options) )
>   {
>       IGUIComboBox::serializeAttributes(out,options);
494,496c491,502
<   out->addEnum ("HTextAlign", HAlign, GUIAlignmentNames);
<   out->addEnum ("VTextAlign", VAlign, GUIAlignmentNames);
<   out->addInt("MaxSelectionRows", (s32)MaxSelectionRows );
---
>       out->addEnum ("HTextAlign", HAlign, GUIAlignmentNames);
>       out->addEnum ("VTextAlign", VAlign, GUIAlignmentNames);
>       out->addInt("MaxSelectionRows", (s32)MaxSelectionRows );
> 
>       out->addInt ("ItemCount",   Items.size());
>       for (u32 i=0; i < Items.size(); ++i)
>       {
>           core::stringc s = "Item";
>           s += i;
>           s += "Text";
>           out->addString(s.c_str(), Items[i].Name.c_str());
>       }
498,505d503
<   out->addInt ("Selected",    Selected );
<   out->addInt ("ItemCount",   Items.size());
<   for (u32 i=0; i < Items.size(); ++i)
<   {
<       core::stringc s = "Item";
<       s += i;
<       s += "Text";
<       out->addString(s.c_str(), Items[i].Name.c_str());
506a505,507
> 
>   // Options enabled for scripting lang
>   out->addInt ("Selected",    Selected );
513c514,519
<   IGUIComboBox::deserializeAttributes(in,options);
---
>   u32 c = 0;
> 
>   // Options disabled for scripting lang
>   if ( !io::isReadWriteForScriptingLang(options) )
>   {
>       IGUIComboBox::deserializeAttributes(in,options);
515,529c521,536
<   setTextAlignment( (EGUI_ALIGNMENT) in->getAttributeAsEnumeration("HTextAlign", GUIAlignmentNames),
<                       (EGUI_ALIGNMENT) in->getAttributeAsEnumeration("VTextAlign", GUIAlignmentNames));
<   setMaxSelectionRows( (u32)(in->getAttributeAsInt("MaxSelectionRows")) );
< 
<   // clear the list
<   clear();
<   // get item count
<   u32 c = in->getAttributeAsInt("ItemCount");
<   // add items
<   for (u32 i=0; i < c; ++i)
<   {
<       core::stringc s = "Item";
<       s += i;
<       s += "Text";
<       addItem(in->getAttributeAsStringW(s.c_str()).c_str(), 0);
---
>       setTextAlignment( (EGUI_ALIGNMENT) in->getAttributeAsEnumeration("HTextAlign", GUIAlignmentNames),
>                         (EGUI_ALIGNMENT) in->getAttributeAsEnumeration("VTextAlign", GUIAlignmentNames));
>       setMaxSelectionRows( (u32)(in->getAttributeAsInt("MaxSelectionRows")) );
> 
>       // clear the list
>       clear();
>       // get item count
>       c = in->getAttributeAsInt("ItemCount");
>       // add items
>       for (u32 i=0; i < c; ++i)
>       {
>           core::stringc s = "Item";
>           s += i;
>           s += "Text";
>           addItem(in->getAttributeAsStringW(s.c_str()).c_str(), 0);
>       }
531a539
>   // Options enabled for scripting lang
 

CGUIListBox.cpp

Code: Select all

 
693a686,691
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       out->addInt("Selected", Selected);
>       return;
>   }
> 
731a730,735
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       Selected = in->getAttributeAsInt("Selected");
>       return;
>   }
> 
 

CGUITabControl.cpp

Code: Select all

 
1012a999,1003
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>       return;
> 
> 
1024,1025c1015,1019
<   Border          = in->getAttributeAsBool("Border");
<   FillBackground  = in->getAttributeAsBool("FillBackground");
---
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       setActiveTab(in->getAttributeAsInt("ActiveTab"));
>       return;
>   }
1027a1022,1024
> 
>   Border          = in->getAttributeAsBool("Border");
>   FillBackground  = in->getAttributeAsBool("FillBackground");
 

CGUITable.cpp

Code: Select all

 
1142a1138,1144
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       out->addInt("ActiveRow", getSelected());
>       out->addInt("ActiveColumn", getActiveColumn());
>       return;
>   }
> 
1209a1212,1218
> 
>   if ( io::isReadWriteForScriptingLang(options) )
>   {
>       setSelected(in->getAttributeAsInt("ActiveRow"));
>       setActiveColumn(in->getAttributeAsInt("ActiveColumn"), in->getAttributeAsBool("DoOrder"));
>       return;
>   }
 

[chronologicaldot]

And just FYI, I made diffs based on comparison with checkout revision 5641.

[CuteAlien]

EARWF_FOR_SCRIPTING_LANG_EXTRA means: no parent serialization? And the problem you have is that all serialization values are changed at once? So this is basically about having a faster solution for part of the variables? And then you offer 2 different interfaces in your script? Admittably a problem in Irrlicht as c++ variables are only connected via those serialization functions to attributes so can't be changed per variable and I suppose it might allow to speed up things somewhat. Anyway - if that's what it's about and it's really useful - I suppose it can be added (edit: thought not too happy about it the more I think about it as it smells a lot like a workaround which is slightly confusing and won't really make a big difference in speed, so basically adding code which will confuse the next maintainer). But needs a better name to explain what it does. And then we have to add it everywhere, not just UI (I can do that part, but please think of a better name which explains what it does).

One slight problem I see with that is we have to think about how to handle elements with a third class layer (particle emitters are a candidate for that and will get that 3rd layer one day as they copy-paste too much shared code, just didn't do that yet as I can't decide between adding a struct or adding another base-class).

EARWF_FOR_SCRIPTING_LANG feels random, I don't get that one so far (maybe have to read again, no more time right now ...).

[chronologicaldot]

Yeah, mostly. Actually, it's a matter of avoiding the issue of having to reset variables all the time, not just about speed. For example, when you work with a checkbox in a scripting language, chances are - after initial setup - the only value you'll ever want to change or check is "Checked". But in order to set that value, you have to remember to set all the other values. If you forget, the element disappears because its values get defaults (Visible = false, Enabled = false, RelativeRect = rect(0,0,0,0), etc). However, some elements have way too many other things related to the element, hence the separation of the basic LANG and EXTRA. EXTRA actually meant "get the extra variables associated with the element", which also happens to mean excluding parent variable values.

I see what you mean with the 3rd layer issue. I'll have to look into that.

I picked the name "EARWF_FOR_SCRIPTING_LANG" just because it's targeted for that purpose and "EARWF_FOR_EDITOR" had weird stuff associated with it.

EDIT: But yeah, I'll try to think of a better name than EXTRA or at least better documentation text.

[CuteAlien]

We can maybe do a better fix - which we already started doing in some places. The IAttributes get functions in svn trunk allow passing a default value. So it's possible (and actually a good idea, I was just too lazy so far...) to simply pass the old value as default parameter. So in other words - any attribute which is not set would keep it's old value and you don't have to set them all anymore.

So for example instead of

Code: Select all

 
setDrawBorder( in->getAttributeAsBool("Border") );
 

it can now be

Code: Select all

 
setDrawBorder( in->getAttributeAsBool("Border", Border) );
 

Which will simply set old value when "Border" attribute is not part of IAttributes.

That way you can even pass single values in IAttributes to deserializeAttributes. Not sure if it's faster or slower than passing all values (still looks for each value, but when IAttributes only contains a single paramter the failing lookup is likely a lot faster on average).

[chronologicaldot]

I do like that idea, but...
1) The speed depends on the the element being de/serialized. CGUITable does quite a bit of string building in its serialization methods, which is obviously rather slow.
2) On top of that, there are no defaults for post-initialization values like getting/setting "active row" and "active column" in CGUITable (among others), which is something you might want to do on the scripting language side.

Admittedly, CGUITable is a bit of an exception, but there are other elements like CGUIListBox with string-building.

That said, I think implementing the fix you mentioned would eliminate the need for the "EXTRA" flag. I don't mind going through and making that fix for GUI elements. Scene elements aren't top priority for me, but I may get around to that too.

[CuteAlien]

You know you can also always call the getter/setter functions directly if you need speed for just 1-2 functions :-) (but I suppose having 1-2 exceptions in the interface would make for an ugly interface)

[chronologicaldot]

I considered that, but at the time, the problem was the unsafeness of type-checking (since everything is made via the registered GUI factories, which default to EGUIET_ELEMENT for anything without a "standard interface" (e.g. IGUICheckBox)). That's also what brought up the GUI tabs issue (using CGUITab instead of IGUITab) alittle while back. And yeah, casting to use 1-2 functions would make for inconsistent interface.

To be less ambiguous, perhaps "EARWF_FOR_SCRIPTING_LANG" should be renamed "EARWF_FREQUENTLY_CHANGED_ATTRS" since that's what it's meant for.

[CuteAlien]

Sorry, CGUITab thing is still on my todo. Currently tricky for me to find time for Irrlicht, so I still haven't reviewed that one.

[chronologicaldot]

Bump. I asked about a better name, so I've been waiting on feedback in this regard.
Anyone else want to pitch in?

[chronologicaldot]

Thanks for getting around to these!

Looks like you overlooked IGUIElement though.
Here's my copy of IGUIElement::deserializeAttributes, slightly rearranged because of what I was doing.

Code: Select all

 
    virtual void deserializeAttributes(io::IAttributes* in, io::SAttributeReadWriteOptions* options=0)
    {
        setID(in->getAttributeAsInt("Id", ID));
        setVisible(in->getAttributeAsBool("Visible", IsVisible));
        setEnabled(in->getAttributeAsBool("Enabled", IsEnabled));
        //if ( io::isReadWriteForOftenCheckedAttrs(options) ) // You don't need this.
        //  return;
 
        setName(in->getAttributeAsString("Name", Name));
        setText(in->getAttributeAsStringW("Caption", Text.c_str()).c_str());
        setToolTipText(in->getAttributeAsStringW("ToolTip", ToolTipText.c_str()).c_str());
        IsTabStop = in->getAttributeAsBool("TabStop", IsTabStop);
        IsTabGroup = in->getAttributeAsBool("TabGroup", IsTabGroup);
        TabOrder = in->getAttributeAsInt("TabOrder", TabOrder);
 
        core::rect<s32> lastDesiredRect = DesiredRect;
 
        core::position2di p = in->getAttributeAsPosition2d("MaxSize", core::position2di(MaxSize.Width, MaxSize.Height));
        setMaxSize(core::dimension2du(p.X,p.Y));
 
        p = in->getAttributeAsPosition2d("MinSize", core::position2di(MinSize.Width, MinSize.Height));
        setMinSize(core::dimension2du(p.X,p.Y));
 
        s32 left_align = in->getAttributeAsEnumeration("LeftAlign", GUIAlignmentNames),
            right_align = in->getAttributeAsEnumeration("RightAlign", GUIAlignmentNames),
            top_align = in->getAttributeAsEnumeration("TopAlign", GUIAlignmentNames),
            bottom_align = in->getAttributeAsEnumeration("BottomAlign", GUIAlignmentNames);
 
        setAlignment((left_align != -1)? (EGUI_ALIGNMENT)left_align : AlignLeft,
                    (right_align != -1)? (EGUI_ALIGNMENT)right_align : AlignRight,
                    (top_align != -1)? (EGUI_ALIGNMENT)top_align : AlignTop,
                    (bottom_align != -1)? (EGUI_ALIGNMENT)bottom_align : AlignBottom);
 
        setRelativePosition(in->getAttributeAsRect("Rect", lastDesiredRect));
 
        setNotClipped(in->getAttributeAsBool("NoClip", NoClip));
    }
 

I may have lost some formatting after the paste.

[CuteAlien]

Thanks, I missed that one. Was there some reason you did the lastDesiredRect in an extra variable? (maybe something about order of serialization messing up or so?)