MyGUI / mygui

Fast, flexible and simple GUI.
http://mygui.info/
Other
723 stars 206 forks source link

MYGUI_DONT_USE_OBSOLETE=ON: Progress bars are not filled #214

Closed glebm closed 1 year ago

glebm commented 3 years ago

After upgrading to MyGUI 3.4.1, bars like the HP bar here in the lower left corner are no longer filled:

image

Health bar references:

Debugger output:

3.4.1:

MyGUI::UserData = {
      mMapUserString = size=0 {}
      mUserData = (mContent = 0x0000000000000000)
      mInternalData = (mContent = 0x0000000000000000)
    }

3.4.0:

MyGUI::UserData = {
      mMapUserString = size=3 {
        [0] = (first = "HeightLine", second = "18")
        [1] = (first = "TrackFill", second = "1")
        [2] = (first = "TrackSkin", second = "MW_BarTrack_Red")
      }
      mUserData = (mContent = 0x0000000000000000)
      mInternalData = (mContent = 0x0000000000000000)
    }

OpenMW issue: https://gitlab.com/OpenMW/openmw/-/issues/5896

Not sure whether this is a bug in OpenMW or MyGUI.

akortunov commented 3 years ago

Nothing changed from OpenMW side - we still fill mMapUserString via Property keys in layout files.

Altren commented 3 years ago

I don't see any related changes, so I believe it would be good to try bisecting the issue, by using intermediate MyGUI versions. Is it possible?

glebm commented 3 years ago

I'll give it a go

glebm commented 3 years ago

Huh, this issue is also present in 3.4.0 when compiled from source. CMake settings that we compile it with: https://gitlab.com/OpenMW/openmw/-/blob/89aed67e2be9eac4f86fb02e32644e452b011916/extern/CMakeLists.txt#L53-58

glebm commented 3 years ago

It's the MYGUI_DONT_USE_OBSOLETE flag

glebm commented 3 years ago

Any idea what the obsolete thing that OpenMW is using that's causing this?

Altren commented 3 years ago

It disables support of deprecated/renamed properties and compilation of deprecated API's. It seems that some old name of a property is used in the layout/skin/etc. Checking the MyGUI.log might help, because it usually (but not always) warns about usage of such strings.

Altren commented 3 years ago

You need to have MYGUI_DONT_USE_OBSOLETE off to see the warnings, though.

glebm commented 3 years ago

There's nothing in the log from MyGUI

glebm commented 3 years ago

Does anything look obsolete here?

    <Resource type="ResourceSkin" name="MW_EnergyBar_Red" size="64 12">
        <Property key="TrackSkin" value="MW_BarTrack_Red"/>
        <Property key="TrackFill" value="1"/>

        <Child type="Widget" skin="MW_Box" offset="0 0 64 12" align="Stretch"/>
        <Child type="Widget" skin="BlackBG" offset="2 2 60 8" align="Stretch" name="Client"/>
    </Resource>
glebm commented 3 years ago

I see some ignored properties


        mPropertyIgnore.insert("TrackSkin");
        mPropertyIgnore.insert("TrackWidth");
        mPropertyIgnore.insert("TrackMin");
        mPropertyIgnore.insert("TrackStep");
        mPropertyIgnore.insert("TrackFill");
        mPropertyIgnore.insert("TrackRangeMargins");

What is the appropriate replacement?

Altren commented 3 years ago

Looking at the mPropertyIgnore it seems, that is is used wrong. Don't understand why it is used this way:

const MapString& properties = _skinInfo->getProperties();
for (MapString::const_iterator item = properties.begin(); item != properties.end(); ++item)
{
    if (BackwardCompatibility::isIgnoreProperty((*item).first))
        setUserString((*item).first, (*item).second);
}

And isIgnoreProperty always return false, when MYGUI_DONT_USE_OBSOLETE is on.

Altren commented 3 years ago

mPropertyIgnore have misleading name, and it is also used to not warn about some properties, because those are known and not deprecated.

glebm commented 3 years ago

Thanks for the investigation! Will you look into fixing this?

faerietree commented 3 years ago

Would be great :)

Altren commented 3 years ago

I tried to reproduce the issue, but failed, nor I can't find the reason of your issue. To make sure, that the BackwardCompatibility::isIgnoreProperty is related could you try to change its non-obsolete implementation from return false; to return true; and see if the issues is still there.

Assumeru commented 1 year ago

Changing that line from false to true indeed fixes the issue.

Altren commented 1 year ago

I figured out what is the issue: some properties that used to be part of the skin were moved into UserString field of the widget property, for example that's how progress bar is defined in one of default skins:

    <Resource type="ResourceLayout" name="ProgressBarFill" version="3.2.0">
        <Widget type="Widget" skin="ScrollPanelHSkin" position="25 50 125 15" name="Root">
            <UserString key="LE_TargetWidgetType" value="ProgressBar"/>
            <UserString key="TrackFill" value="true"/>
            <UserString key="TrackSkin" value="ProgressBarTrackHSkin"/>
            <UserString key="TrackWidth" value="6"/>
            <Widget type="Widget" skin="PanelEmpty" position="2 1 122 14" align="Stretch" name="TrackPlace">
                <Property key="NeedKey" value="false"/>
                <Property key="InheritsPick" value="true"/>
            </Widget>
        </Widget>
    </Resource>

So to properly fix an issue you need to use ResourceLayout for the widget skin if you also want to avoid using deprecated logic (i.e. build with MYGUI_DONT_USE_OBSOLETE)

Altren commented 1 year ago

Also there is a warning, that states that there is something wrong with the skin:

Widget '|ProgressBar' have unknown property 'TrackFill'  [test.layout]
Widget '|ProgressBar' have unknown property 'TrackSkin'  [test.layout]
akortunov commented 1 year ago

I figured out what is the issue: some properties that used to be part of the skin were moved into UserString field of the widget property,

EDIT: we will try to use UserString.

Also there is a warning, that states that there is something wrong with the skin

Yes, these warnings are related to this issue.

Altren commented 1 year ago

EDIT: we will try to use UserString.

Note: not just UserString -> Property replacement. You need to use ResourceLayout instead of just ResourceSkin. Then the "Root" named widget should have those UserStrings. You can use skins from the MyGUI as a reference

akortunov commented 1 year ago

What to do with Child nodes?

    <Resource type="ResourceSkin" name="MW_Progress_Drowning_Full" size="64 6">
        <Property key="TrackSkin" value="MW_Progress_Drowning_Full_Small"/>
        <Property key="TrackFill" value="1"/>
        <Child type="Widget" skin="TransparentBG" offset="2 2 60 2" align="Stretch" name="Client"/>
    </Resource>

ResourceLayout seems to ignore them.

Altren commented 1 year ago

Here's corrected version:

    <Resource type="ResourceLayout" name="MW_EnergyBar_Red" size="64 12">
        <Widget type="Widget" skin="MW_Box" position="0 0 64 12" name="Root" align="Stretch">
            <UserString key="TrackSkin" value="MW_BarTrack_Red"/>
            <UserString key="TrackFill" value="true"/>
            <Widget type="Widget" skin="BlackBG" offset="2 2 60 8" align="Stretch" name="TrackPlace"/>
                <Property key="NeedKey" value="false"/>
                <Property key="InheritsPick" value="true"/>
            </Widget>
        </Widget>
    </Resource>
akortunov commented 1 year ago

Here's corrected version

  1. There is an XML syntax error (should be > instead of />) in this sample.
  2. Root widget ignores a skin property, so MW_Box seems to need a separate inner widget.

A code which works for me with 3.4.2:

    <Resource type="ResourceLayout" name="MW_EnergyBar_Red" size="64 12">
        <Widget type="Widget" position="0 0 64 12" name="Root">
            <UserString key="TrackSkin" value="MW_BarTrack_Red"/>
            <UserString key="TrackFill" value="true"/>
            <Widget type="Widget" skin="BlackBG" position="2 2 60 8" align="Stretch" name="TrackPlace">
                <Property key="NeedKey" value="false"/>
                <Property key="InheritsPick" value="true"/>
            </Widget>
            <Widget type="Widget" skin="MW_Box" position="0 0 64 12" align="Stretch" name="Border">
                <Property key="NeedKey" value="false"/>
                <Property key="InheritsPick" value="true"/>
            </Widget>
        </Widget>
    </Resource>
Altren commented 1 year ago

Sorry for the typo. Not sure why skin is ignored in your case, maybe its visuals is simply covered by something else. But your varint is also ok.

akortunov commented 1 year ago

Also I checked MyGUI_WidgetInput.h, and NeedKey flag is already false by default. Not sure what InheritsPick is supposed to do and why we need it.

maybe its visuals is simply covered by something else

No, the space is free. Border just is not rendered, as if there is no skin node for Root widget.

Altren commented 1 year ago

NeedKey and InheritsPick are really not necessary here, not sure why it was added in the original skin. You might want to add NeedMouse false on child widgets in case you want to handle mouse interaction, but this is not mandatory as well.