Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ipochto/highground fixes #581

Merged
merged 30 commits into from
Nov 28, 2023
Merged

Ipochto/highground fixes #581

merged 30 commits into from
Nov 28, 2023

Conversation

ipochto
Copy link
Member

@ipochto ipochto commented Nov 24, 2023

  • Some fixes to make highgrounds-related wargus's scripts works

src/editor/editloop.cpp Outdated Show resolved Hide resolved
src/guichan/include/guichan/listmodel.h Outdated Show resolved Hide resolved
src/include/map.h Outdated Show resolved Hide resolved
src/include/map.h Outdated Show resolved Hide resolved
src/include/script.h Outdated Show resolved Hide resolved
src/map/mapfield.cpp Outdated Show resolved Hide resolved
src/map/script_map.cpp Outdated Show resolved Hide resolved
src/map/script_map.cpp Outdated Show resolved Hide resolved
src/map/script_map.cpp Show resolved Hide resolved
src/map/script_map.cpp Outdated Show resolved Hide resolved
@ipochto
Copy link
Member Author

ipochto commented Nov 25, 2023

@Jarod42, Sorry I'm not so familiar with github's routine, so it turn out that I'm fixed your suggestions twise - via github and locally.

@ipochto ipochto force-pushed the ipochto/highground-fixes branch 2 times, most recently from c27a702 to c936051 Compare November 27, 2023 14:04
@ipochto ipochto requested a review from Jarod42 November 27, 2023 15:38
inline int LuaGetArgsNum(lua_State *l)
{
return lua_gettop(l);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For next times, don't hesitate to split your PR.
That change might be a dedicated PR, is unrelated to HighgroundsEnabled feature.

Comment on lines +1092 to +1103
static int CclMapEnableHighgrounds(lua_State *l)
{
Map.Info.EnableHighgrounds(LuaGetArgsNum(l) >= 1 ? LuaToBoolean(l, 1) : true);

return 0;
}

static int CclIsHighgroundsEnabled(lua_State *l)
{
lua_pushboolean(l, Map.Info.IsHighgroundsEnabled());
return 1;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might probably go in map.pkg (tolua++) instead.

Copy link
Member Author

@ipochto ipochto Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference in principle?

I just did it by analogy, depending on how existing functions with similar functionality were translated into lua. If, for example, what concerns gui was translated through 'tolua++', then almost everything I've encountered concerning engine settings or parameters of the current game was done through lua_register() .

To change the method is not a problem. I just want to understand for the future on what principle to choose the translation method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure neither ;-)

to_lua has been added after.

BTW, the flag seems unused in the Engine :-/ ...
Is there incoming PRs next?
How does it work?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It used here: Wargus/wargus#446

Copy link
Member Author

@ipochto ipochto Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to this flag we choose to load base tileset or run extended tileset generator to make tiles for highgrounds/ramps/cliffs etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently

      Map.Info.Postamble    = ""
      MapEnableHighgrounds(highgroundsCheckBox:isMarked())

With Tolua, it would be more consistent:

      Map.Info.Postamble    = ""
      Map.Info.HighgroundsEnabled = highgroundsCheckBox:isMarked()

but it seems that Highgrounds is more related to tileset than map.
wonder if it is then the right flag/place.
Shouldn't we choose between summer/summer_highgrounds/swamp/swamp_highgrounds...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map.Info.HighgroundsEnabled = highgroundsCheckBox:isMarked()

This is a private member and I don't want to open it in case if we will need to do something else with enabling it in the future. At least the network synchronization may be required.

I could use Tolua to open MapEnableHighgrounds()/IsHighgroundsEnabled(), but definitely don't want to make Map.Info.HighgroundsEnabled variable as public.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we choose between summer/summer_highgrounds/swamp/swamp_highgrounds...?

Don't breed entities. )

The player simply selects the basic tileset as before, and wargus chooses whether to load the extended version or not. Moreover, if wargus "sees" that the map contains highgrounds, it will simply hide the possibility to select (in dropDownList) the base tileset, for which there is no possibility to generate the extended version (I am talking about swamp specifically - there are certain difficulties with generation).

For the editor, there is a checkbox to enable highgrounds or not.

So the Map.Info.HighgroundsEnabled is map's property, on the basis of which the choice of which tileset to load is made

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the map presentation is loaded before tileset. Tileset loads with game start, but map presentation is uploaded to the stage of choosing the game parameters by the player (in the create a new game menu).

----------------------------------------------------------------------------*/

int StringListModel::getIdxOfElement(std::string_view element)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int StringListModel::getIdxOfElement(std::string_view element)
int StringListModel::getIdxOfElement(std::string_view element) const

@@ -1735,6 +1750,16 @@ void LuaListModel::setList(lua_State *lua, lua_Object *lo)
}
}

int LuaListModel::getIdxOfElement(std::string_view element)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int LuaListModel::getIdxOfElement(std::string_view element)
int LuaListModel::getIdxOfElement(std::string_view element) const

@@ -2610,6 +2635,13 @@ int ImageDropDownWidget::getSelected()
return mListBox.getSelected();
}

std::string ImageDropDownWidget::getSelectedItem()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string ImageDropDownWidget::getSelectedItem()
std::string ImageDropDownWidget::getSelectedItem() const

@Jarod42 Jarod42 merged commit e44db04 into master Nov 28, 2023
4 checks passed
@Jarod42 Jarod42 deleted the ipochto/highground-fixes branch November 28, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants