Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upNew callbacks for LUA #21744
Conversation
This was referenced Aug 29, 2017
This comment has been minimized.
This comment has been minimized.
|
You might want to consider passing the various arguments to the lua callbacks so that the lua code doesn't have to try and figure things out manually. |
This comment has been minimized.
This comment has been minimized.
Thanks for the suggestion. I've also considered this - some of the callbacks will greatly benefit from this (like, immediately knowing which stat was changed or which item was taken off will make lua-modding simplier). I've already started looking into it, but it will only make sense if anything new regarding lua will be ever merged. |
ZhilkinSerg
changed the title
[CR] Several callbacks for LUA
[CR] New callbacks for LUA
Aug 30, 2017
BevapDin
reviewed
Aug 30, 2017
| /** | ||
| * Execute a callback that can be overriden by all mods with 2 argument(s) put to global table (_G). | ||
| */ | ||
| void lua_callback( const char *callback_name, const char *callback_arg1, const char *callback_arg2 ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Aug 30, 2017
Contributor
Variables / parameters with numbers in them are a code smell - use a vector or similar instead. You don't need n callback functions, only one generic.
Even better would be a function template, so the caller doesn't need to convert everything into a string, which the Lua code has to convert back into whatever it actually is.
This comment has been minimized.
This comment has been minimized.
ZhilkinSerg
Aug 30, 2017
•
Author
Contributor
Variables / parameters with numbers in them are a code smell - use a vector or similar instead. You don't need n callback functions, only one generic.
Even better would be a function template, so the caller doesn't need to convert everything into a string, which the Lua code has to convert back into whatever it actually is.
I've tried using cstdarg and its ..., but didn't achieve good results - if I understood it right, all of the parameters should be of the same type, but it will be required to have multiple types in a single callback. Same goes for using vector - it can only contain variables of single type.
I've decided to use string for test purposes for the time being as it nicely fits for various numbers and string identifiers, but I will definitely need to rework this as I will need references to certain objects.
I didn't work with function templates previously, but I believe it is something about preprocessor magic, right?
This comment has been minimized.
This comment has been minimized.
Very strange - build compiles via MSYS2 in Code::Blocks on Windows locally without such issues... |
This comment has been minimized.
This comment has been minimized.
|
So, following code compiles, runs and works perfectly when, but when same code is put to
#include <iostream>
template<typename ArgType> void lua_callback_store_arg(
const int callback_arg_idx, ArgType callback_arg )
{
const char *callback_arg_name = std::string( "callback_arg" + std::to_string(
callback_arg_idx ) ).c_str();
std::cout << callback_arg_name << ": " << callback_arg << std::endl;
}
void lua_callback_store_args( const int callback_arg_idx )
{
std::cout << "callback_arg_count: " << callback_arg_idx - 1 << std::endl;
}
template<typename ArgType, typename... Args> void lua_callback_store_args(
const int callback_arg_idx, ArgType callback_arg,
Args... callback_args )
{
lua_callback_store_arg( callback_arg_idx, callback_arg );
lua_callback_store_args( callback_arg_idx + 1, callback_args... );
}
void lua_callback( const char *callback_name )
{
std::cout << "callback_last: " << callback_name << std::endl;
}
template<typename ... Args> void lua_callback( const char *callback_name, Args... callback_args )
{
lua_callback_store_args( 1, callback_args... );
lua_callback( callback_name );
}
int main()
{
lua_callback( "callbacktest", 1, 3, '1', "234234", 1.67f );
lua_callback( "callbacktest2" );
lua_callback( "callbacktest3", "test" );
} |
This comment has been minimized.
This comment has been minimized.
|
It's rather massive, padded due to the styling.
|
This comment has been minimized.
This comment has been minimized.
Yes, because that function template is never instantiated as it's only defined in the cpp file. You can force an instantiation by adding this line to the cpp file: template void lua_callback<const char*>( const char*, const char* ); |
This comment has been minimized.
This comment has been minimized.
template void lua_callback<const char*>( const char*, const char* );Thanks, it really helped. |
ZhilkinSerg
changed the title
[CR] New callbacks for LUA
[WIP] New callbacks for LUA
Oct 3, 2017
BevapDin
reviewed
Oct 4, 2017
| // possible callback arguments: skill_increase_type, skill_id, skill_level_new | ||
| const static auto skill_increase_type = "training"; | ||
| const static auto skill_id = skill.ident().c_str(); | ||
| const static auto skill_level_new = std::to_string( new_skill_level ).c_str(); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Oct 4, 2017
Contributor
This creates a dangling pointer. to_string returns a value that only exists until the end of the expression. The pointer from c_str is invalid after that. Why don't you put all those values directly into the function call instead of having several variables that are used only once anyway?
This comment has been minimized.
This comment has been minimized.
ZhilkinSerg
Oct 4, 2017
•
Author
Contributor
Thanks, will change that.
That is just a stub, btw. I'm trying to figure out convinient way of transferring callback arguments (which come in different type and numbers) to lua. I'm not sure yet what is the best approach.
BevapDin
reviewed
Oct 4, 2017
| // possible callback arguments: skill_increase_type, skill_id, skill_level_new | ||
| const static auto skill_increase_type = "book"; | ||
| const static auto skill_id = skill.c_str(); | ||
| const static auto skill_level_new = std::to_string( originalSkillLevel + 1 ).c_str(); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Oct 4, 2017
Contributor
Same here. Btw. please use to_string without the std:: prefix. See "src/compatibility.h", some systems seem to have trouble with std::to_string (it is called without the prefix throughout the code).
BevapDin
reviewed
Oct 4, 2017
| // possible callback arguments: skill_increase_type, skill_id, skill_level_new | ||
| const static auto skill_increase_type = "practice"; | ||
| const static auto skill_id = skill.ident().c_str(); | ||
| const static auto skill_level_new = std::to_string( newLevel ).c_str(); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Still struggling with Travis... The error is the following:
I've added following code to template void lua_callback<const char *>( const char *, const char * );
template void lua_callback<const char *>( const char *, const char *, const char * );
template void lua_callback<const char *>( const char *, const char *, const char *, const char * ); |
This comment has been minimized.
This comment has been minimized.
Look carefully at the error message. The linker expected a function named template void lua_callback<const char *>( const char *, const char * );
template void lua_callback<const char *, const char *>( const char *, const char *, const char * );
template void lua_callback<const char *, const char *, const char *>( const char *, const char *, const char *, const char * );And it may be required to put those lines after the definition of |
This comment has been minimized.
This comment has been minimized.
Thanks a lot! That was the case. My fault was that I tried to compile only LUA build locally and skipped non-LUA. The latter one failed to build on my workstation too until I moved template instantiation to block shared between both builds. |
BevapDin
reviewed
Mar 18, 2018
| const int callback_arg_idx, ArgType callback_arg ) | ||
| { | ||
| const char *callback_arg_name = std::string( "callback_arg" + std::to_string( | ||
| callback_arg_idx ) ).c_str(); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 18, 2018
Contributor
You should probably avoid all the raw C-strings and use std::string (and for parameters const std::string &) instead.
The usage of callback_arg_name below invokes undefined behaviour as it is no longer a valid pointer. c_str here is invoked on a temporary object and the result is only valid as long as that std::string object stays unchanged, but it's deconstructed at the end of this expression.
This comment has been minimized.
This comment has been minimized.
ZhilkinSerg
Mar 18, 2018
Author
Contributor
There were some issues with ellipsis ... and non c_str, so I used it everywhere. Anyway I decided to refactor the way attributes are transfered to callbacks.
BevapDin
reviewed
Mar 18, 2018
| lua_callback("on_minute_passed"); | ||
| } | ||
|
|
||
| // Run a LUA callback once per turn | ||
| if( calendar::once_every( 1_turns ) ) { |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 18, 2018
Contributor
This check is a bit pointless as it will be true all the time. (game::do_turn is called once for each turn, it actually increments the turn counter.)
BevapDin
reviewed
Mar 18, 2018
| } else { | ||
| add_msg( m_good, _( "%s increases their %s level." ), learner->disp_name().c_str(), | ||
| skill.obj().name().c_str() ); | ||
| skill.obj().name().c_str(), skill.obj().name().c_str() ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 18, 2018
Contributor
The message foprmat string only contains two "%s", and there are already two arguments for thos placeholders. Why have you added a third argument (identicaly to the second one), that is never used?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Is it CLANG only warning (
I do not have error during compilation or in runtime when compiled in Visual Studio and it works pretty good: |
ZhilkinSerg
force-pushed the
ZhilkinSerg:lua-callbacks
branch
from
263fea8
to
e0fa5ae
Mar 19, 2018
This comment has been minimized.
This comment has been minimized.
|
Haha! And it finally compiles in Jenkins, Travis and locally. Thanks to @BevapDin - you past suggestions and feedback helped me a lot. I really appreciate this! Output from `\config\dda-lua-test-callback.log`
|
ZhilkinSerg
changed the title
[WIP] New callbacks for LUA
New callbacks for LUA
Mar 19, 2018
BevapDin
reviewed
Mar 21, 2018
|
|
||
| lua_callback("on_skill_increased"); | ||
| CallbackArgumentContainer lua_callback_args_info; | ||
| lua_callback_args_info.emplace_back( CallbackArgument( "skill_increased_source", "training" ) ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 21, 2018
Contributor
When you use one of the emplace functions of a standard container, you don't need to create an object of the contained type. The emplace functions construct such an object on their own and forward all there arguments to its constructor.
In short: some_container.emplace_back( 1, 2, 3, 4 ) is (mostly) the same as some_container.push_back( Type( 1, 2, 3, 4 ) ).
BevapDin
reviewed
Mar 21, 2018
| lua_delete_global( "mutation_gained" ); | ||
| lua_delete_global( "mutation_lost" ); | ||
| lua_delete_global( "stat_changed" ); | ||
| lua_delete_global( "stat_value" ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 21, 2018
Contributor
This looks wrong. Why should there be a global list of all possible parameters to the callback functions? What if someone adds a new callback and forgets to add the parameter name here?
Your CallbackArgumentContainer type already knows the names of all parameters, so it can be used to remove only those variables in Lua that were actually set up.
This comment has been minimized.
This comment has been minimized.
ZhilkinSerg
Mar 21, 2018
Author
Contributor
Actually, I want to clean existing callback arguments whenever new callback is raised. Maybe I should simply create a table in _G named callback_arguments, store all callback arguments there and clean this table?
BevapDin
reviewed
Mar 21, 2018
| * Execute a callback that can be overridden by all mods, | ||
| * storing provided callback arguments in _G. | ||
| */ | ||
| void lua_callback( const char *callback_name, CallbackArgumentContainer callback_args ); |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 21, 2018
Contributor
Why does this function take a copy of the CallbackArgumentContainer object and not just a const reference?
BevapDin
reviewed
Mar 21, 2018
| std::string GetValueString() { | ||
| return value_string; | ||
| } | ||
| tripoint GetValueTripoint() { |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 21, 2018
Contributor
All these getter functions are non-const, why? They don't change the object in any way.
BevapDin
reviewed
Mar 21, 2018
| type = "double"; | ||
| value_double = arg_value; | ||
| }; | ||
| CallbackArgument( std::string arg_name, float arg_value ) { |
This comment has been minimized.
This comment has been minimized.
BevapDin
Mar 21, 2018
Contributor
Lua does not have separate float and double types. Creating a CallbackArgument with a float will behave the same as one with a double (in both cases lua_pushnumber is called).
ZhilkinSerg
added
the
(P3 - Medium)
label
Apr 2, 2018
ZhilkinSerg
removed
the
(P3 - Medium)
label
Apr 18, 2018
This comment has been minimized.
This comment has been minimized.
|
@BevapDin, I've commented in your |
ZhilkinSerg
added
the
(S5 - OnHold / Stalled)
label
Jul 12, 2018
ZhilkinSerg
closed this
Jul 12, 2018
ZhilkinSerg
reopened this
Sep 6, 2018
ZhilkinSerg
force-pushed the
ZhilkinSerg:lua-callbacks
branch
from
e7d09df
Sep 6, 2018
ZhilkinSerg
closed this
Sep 6, 2018
This comment has been minimized.
This comment has been minimized.
|
Do you still plan to mainline these callbacks? I would like to use these. |
This comment has been minimized.
This comment has been minimized.
|
Yes. They will be mainlined, but I wanted to refactor something first. |
ZhilkinSerg
reopened this
Sep 9, 2018
ZhilkinSerg
closed this
Sep 9, 2018
ZhilkinSerg
reopened this
Sep 10, 2018
ZhilkinSerg
force-pushed the
ZhilkinSerg:lua-callbacks
branch
to
83b4caa
Sep 10, 2018
ZhilkinSerg
added some commits
Sep 10, 2018
ZhilkinSerg
force-pushed the
ZhilkinSerg:lua-callbacks
branch
from
8bdf49e
to
2959ace
Sep 10, 2018
ZhilkinSerg
removed
the
(S5 - OnHold / Stalled)
label
Sep 10, 2018
ZhilkinSerg
added
<Documentation>
Mods
labels
Sep 10, 2018
ZhilkinSerg
merged commit 17101c4
into
CleverRaven:master
Sep 10, 2018
This comment has been minimized.
This comment has been minimized.
|
good job! but i am a little concerned about player-related callback runs when npc satisfies conditions. |
This comment has been minimized.
This comment has been minimized.
These are player-specific callbacks which should not affect npcs. Edit: You were right though - callbacks are raised for npcs too. I will update callbacks to be player-only. |
This comment has been minimized.
This comment has been minimized.
|
See #25540 for follow-up. |
This comment has been minimized.
This comment has been minimized.
|
OH, thx!!! |




ZhilkinSerg commentedAug 29, 2017
•
edited
Summary
SUMMARY: Features "Added more Lua callback (some with arguments)"Purpose of change
Expand Lua-modding possibilites.
Describe the solution
Following Lua-callbacks were added:
game-related:
on_savegame_loadedruns when saved game is loaded;on_weather_changed(weather_new, weather_old)runs when weather is changed.calendar-related:
on_turn_passedruns once each turn (once per 6 seconds or 10 times per minute);on_hour_passedruns once per hour (at the beginning of the hour);on_year_passedruns once per year (on first day of the year at midnight).player-related:
on_player_skill_increased(skill_increased_source, skill_increased_id, skill_increased_level)runs whenever player skill is increased (previously known ason_skill_increased);on_player_dodge(source_dodge,difficulty_dodge)runs whenever player have dodged;on_player_hit(source_hit, body_part_hit)runs whenever player were hit;on_player_hurt(source_hurt, disturb)runs whenever player were hurt;on_player_mutation_gain(mutation_gained)runs whenever player gains mutation;on_player_mutation_loss(mutation_lost)runs whenever player loses mutation;on_player_stat_change(stat_changed,stat_value)runs whenever player stats are changed;on_player_effect_int_changes(effect_changed, effect_intensity, effect_bodypart)runs whenever intensity of effect on player has changed;on_player_item_wear(item_last_worn)runs whenever player wears some clothes on;on_player_item_takeoff(item_last_taken_off)runs whenever player takes some clothes off;on_mission_assignment(mission_assigned)runs whenever player is assigned to mission;on_mission_finished(mission_finished)runs whenever player finishes the mission.mapgen-related:
on_mapgen_finished(mapgen_generator_type, mapgen_terrain_type_id, mapgen_terrain_coordinates)runs wheneverbuiltin,jsonorluamapgen is finished generating.Some callbacks provide arguments which can be useful (see example mod).
Additional context:
I want to utilize this in in my
dda-luamod:STRor lowDEX/INTtries to take off or wear some clothes (for fun);Degrade Buildingsmod, but without need to addluanode to mapgen json).