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

use ref in PROCESSOR_SELECT_SYNCHRO #606

Merged
merged 17 commits into from
Jul 27, 2024
Merged

use ref in PROCESSOR_SELECT_SYNCHRO #606

merged 17 commits into from
Jul 27, 2024

Conversation

salix5
Copy link
Collaborator

@salix5 salix5 commented Jun 26, 2024

Add interpreter::check_filter

check_matching is replaced by check_filter, which takes lua_State* as 1st parameter.

This function is usually called from Lua functions like:

int32 scriptlib::duel_select_matching_cards(lua_State *L);

The arguments are on the stack of L, so the function indices are also the index of L.

Add ptr3, ptr4 to processor_unit

d5accce
Avoid unnecessary pointer casting.

Push pcard in check_tuner_material

A new parameter scard is added to Card.IsNotTuner in 3c46099, and the new parameter is pushed onto the stack before every function call to check_tuner_material orcheck_synchro_material.
It will make the stack tracing very hard.

0101614
Because we have the pointer to L now, we can push the synchro monster pcard is onto the stack of L before check_filter.
It will make the stack tracing easier.

Always take mg first

c752dee
Before:
In duel_select_synchro_material, smat will be ignored if mg is given.
In duel_check_synchro_material, smat will not be ignored if mg is given.

Now:
smat will always be ignored if mg is given.

Remove lua_pop in synchro procedures

Fluorohydride/ygopro#2407
I agree that passing Lua functions by pushing onto the stack is a bad idea too.

Problems in Fluorohydride/ygopro#2406

https://www.lua.org/manual/5.4/manual.html#lua_status
You can call functions only in threads with status LUA_OK. You can resume threads with status LUA_OK (to start a new coroutine) or LUA_YIELD (to resume a coroutine).

Before #582
In order to reuse the arguments on the stack, ygopro used the yielded thread to call new functions.
It is not allowed by Lua, so the assertion of LUA_USE_APICHECK will fail.

After #582
Now we restore current_state after lua_resume returns.
The Lua treads are:
lua_state
rthread (The arguments of coroutine are here)

So we have to use lua_xmove to move the arguments to another stack.

Problems in Fluorohydride/ygopro#2407

Pushing the arguments on the stack of rthread and moving to another stack is harmful.
It will make the stack tracing hard as hell.

The heap corruption still happens after #582 .
I think it is caused by the same problems in Fluorohydride/ygopro#2407

Solution

If we want to use Lua functions or any objects after the coroutine yields:
We can use reference in Lua registry instead.

If we want to call a Lua filter outside the Lua functions:
We can use interpreter::check_condition instead.

Now the functions filter1 and filter2 are passed to select_synchro_material by the reference in registry.
We don't need to use PARAM_TYPE_INDEX anymore.
fix Fluorohydride/ygopro#2407

remove Group.ForEach

edo9300/ygopro-core@de60f79
I have reached the same conclusion.
Group.ForEach is not used in ygopro-scripts now.
It can (and should) be replaced by normal for-loop in Lua.

PARAM_TYPE_INDEX is not used now.


Fluorohydride/ygopro#2406
Fluorohydride/ygopro#2407
Lua線程
lua_state(主線程)
rthread (呼叫coroutine使用)

原本的傳遞參數方式
函數f1 push到rthread的堆疊
lua_xmove移動到主線程
傳遞f1在主線程堆疊的index
手動從主線程的堆疊pop

這是 @mercury233 記錄在2407的問題
這會讓堆疊的管理變得非常麻煩並且容易出錯
我認為同步召喚有時會產生的heap corruption的主要原因是這個

現在改成
傳遞函數都用registry reference
這是每個線程共通的數值
由於沒有手動push這步
也就不再需要手動pop

Group.ForEach
這個函數會有和2407相同的問題
現有的腳本沒有使用
並且他的功能可以由普通的Lua迴圈完成
因此移除這個函數

@mercury233
@purerosefallen

@purerosefallen
Copy link
Collaborator

purerosefallen commented Jun 27, 2024

I would consider this in the next update, because 3 days are not enough for reviewing and testing.

@salix5 salix5 merged commit a3fe63a into master Jul 27, 2024
1 check passed
@salix5 salix5 deleted the patch-matching branch July 27, 2024 03:57
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.

don't use lua_pop in synchro produces
2 participants