Print

Please login or register.
1 Hour 1 Day 1 Week 1 Month Forever
Login with username, password and session length

[lethosor]

I'll just leave a comment:

Code: [Select]

if bits.whole & 0x7F > 0 then --Any non-blocked tree_tile
It should probably be implemented in C++, really, since you get type checking (coords are int16_t?) and loop continue, which would look more like the assembly. I had to flip some of the conditions.

Edit: Actually, Lua supports GOTO, so I could really make my script look like assembly if I wanted.

Yeah, there are some limitations to Lua (not that it's necessarily a bad thing, but code can't always correspond one-to-one). Higher-level languages often limit what "goto" can do, at least compared to assembly. In Lua's case, for instance:

A goto may jump to any visible label as long as it does not enter into the scope of a local variable.

Maybe it should be printed for "gui/gm-editor" and the Lua "printall". Could something be done about allowing tree_tiles to be indexed (beyond the z-level) instead of using "_displace", or is that not possible due to them not being vectors? I'll add this to the issue tracker it if it's possible.

There's a substantial risk of breaking code by doing that (e.g. if existing code relies on all fields returned by pairs() being booleans); I'd say it's better just to document.

Your _displace() note is due to the fact that those fields are 2D arrays implemented as double pointers. The Lua layer doesn't have great support for these - I don't know exactly why, but I suspect it has to do with "references" that are exposed to Lua (like an "<int32: ...>" field you see somewhere) not supporting multiple layers of indirection (to be clear, I'm pretty sure that's a limitation in our implementation, not Lua itself). https://docs.dfhack.org/en/stable/docs/Lua%20API.html#container-references has a brief note that stemmed from https://github.com/DFHack/dfhack/issues/597.

[lethosor]

I updated the first post of this thread with some new chat options. We have a Discord server and an IRC channel on Libera now. (I'm avoiding linking them here in case any links become stale, so check out the first post for links.)

Update: Freenode has effectively been dismantled at this point - user and channel registrations have been wiped, and most previous servers have been split from the network. Sad to see it go. Fortunately, both Discord and Libera are alive and well (the latter has most of the Freenode staff, from my understanding), so those two are our "official" real-time channels for the time being.

Our documentation has a new page listing all of the relevant support channels (which were previously listed on the "Introduction" page, so hopefully this change makes them easier to find): https://docs.dfhack.org/en/latest/docs/Support.html

[Clément]

The assembly seems to use rdx, r8, and r9 for the coordinates. Is this just an optimization?

Or would the real thing be something like:
getPlantAtCoords(unused, unused, x, unused, y, z, &rbp_value, &x_mod_48, &y_mod_48, &rbx_value)

This is a method on Windows, right? It seems to follow the call convention. Parameter are in rcx, rdx, r8, and r9. rcx is "this" if it is a method call. What were you expecting?

[PatrikLundell]

One problem with the 2D arrays requiring usage of _displace() is that Lua (or our usage of it) is capable of describing those matrices only when they're static, but we don't have the means to specify that the first dimension is according to this field, and the second dimension is according to that one. In fact, we can't even describe one dimensional arrays whose length is determined by the value of a specific field (we can still address the elements blindly, C style, but not check the bounds without specific code for checking it [as opposed to code generated from the usage of a specified field]).

[Bumber]

This is a method on Windows, right? It seems to follow the call convention. Parameter are in rcx, rdx, r8, and r9. rcx is "this" if it is a method call. What were you expecting?

Conventional parameters 1 (rdi) and 2 (rsi) aren't used. The calling functions seem to leave their garbage data in them. I would have expected rdi, rsi, rdx for x, y, z.

As a result, it ends up putting extra parameters on the stack. (Assuming they even are parameters, and not just local variables being stored in rsp+ instead of rsp-.) It doesn't even modify rbp or rbx, just restores them to registers from the stack (without popping) at end of the function. The mod_48 values are calculated by the function and later loaded back into registers with each loop iteration. I haven't checked if there's a calling function that uses them.

Spoiler: Some context: (click to show/hide)

Code: (Start of the function:) [Select]

.text:0000000140CF0F90 getPlantAtCoords_140CF0F90 proc near    ; CODE XREF: sub_1401838E0+28C↑p
.text:0000000140CF0F90                                         ; sub_1401838E0+7D0↑p ...
.text:0000000140CF0F90
.text:0000000140CF0F90 arg_0           = qword ptr  8
.text:0000000140CF0F90 arg_8           = dword ptr  10h
.text:0000000140CF0F90 arg_10          = dword ptr  18h
.text:0000000140CF0F90 arg_18          = qword ptr  20h
.text:0000000140CF0F90
.text:0000000140CF0F90                 mov     [rsp+arg_0], rbx
.text:0000000140CF0F95                 mov     [rsp+arg_18], rbp
.text:0000000140CF0F9A                 push    rsi
.text:0000000140CF0F9B                 push    rdi
.text:0000000140CF0F9C                 push    r12
.text:0000000140CF0F9E                 push    r13
.text:0000000140CF0FA0                 push    r14
.text:0000000140CF0FA2                 push    r15

Code: (Part of a function that calls it:) [Select]

.text:00000001408FDEFA                 mov     byte ptr [rbp+120h+Dst], cl
.text:00000001408FDEFD                 lea     r8d, [rcx+12h]  ; Size
.text:00000001408FDF01                 lea     rdx, aYouChopDownThe ; "You chop down the "
.text:00000001408FDF08                 lea     rcx, [rbp+120h+Dst] ; Dst
.text:00000001408FDF0C                 call    sub_14000C780
.text:00000001408FDF11                 nop
.text:00000001408FDF12
.text:00000001408FDF12 loc_1408FDF12:                          ; DATA XREF: .rdata:000000014106DC58↓o
.text:00000001408FDF12                 movzx   r9d, r12w
.text:00000001408FDF16                 movzx   r8d, r15w
.text:00000001408FDF1A                 movzx   edx, di
.text:00000001408FDF1D                 call    getPlantAtCoords_140CF0F90
.text:00000001408FDF22                 test    rax, rax

rdi actually holds a copy of the x coord in this instance, but it's just a register to be restored by the function, since the one in rdx is used.

[Clément]

You must be confusing Windows and SysV (used on Linux, macOS, ...) call conventions. Windows is rcx, rdx, r8, r9, then stack. SysV is rdi, rsi, rdx, rcx, r8, r9 then stack.

See Microsoft documentation or the nice recap on wikipedia.

The four "arguments" you are seeing must be the "shadow store".

The x64 Application Binary Interface (ABI) uses a four-register fast-call calling convention by default. Space is allocated on the call stack as a shadow store for callees to save those registers. [...] The caller must always allocate sufficient space to store four register parameters, even if the callee doesn't take that many parameters. [...] The callee is responsible for dumping the register parameters into their shadow space if needed.

So a function always gets space for four registers but it can do whatever it wants with it.

[Bumber]

Does this look all right?

Code: [Select]

using df::global::world;

df::plant *getPlantAtCoords(int32_t x, int32_t y, int32_t z)
{
    if ( (x / 48) * 48 < 0 )
        return NULL;
    else if ( (x / 48) * 48 >= world->map.x_count )
        return NULL;
    else if ( (y / 48) * 48 < 0 )
        return NULL;
    else if ( (y / 48) * 48 >= world->map.y_count )
        return NULL;
    else if ( !world->map.column_index )
        return NULL;
   
    auto mbc = world->map.column_index[ (x / 48) * 3 ][ (y / 48) * 3 ];
    if ( !mbc )
        return NULL;
   
    int32_t x_mod_48 = x%48;
    int32_t y_mod_48 = y%48;
    for(size_t i = 0 ; i < mbc->plants.size(); i++)
    {
        auto p = mbc->plants[i];
        if ( p->pos.x == x && p->pos.y == y && p->pos.z == z )
            return p;
       
        auto t = p->tree_info;
        if ( !t )
            continue;
       
        int32_t x_index = t->dim_x / 2 - p->pos.x%48 + x_mod_48;
        int32_t y_index = t->dim_y / 2 - p->pos.y%48 + y_mod_48;
        int32_t z_dis = z - p->pos.z;
        if ( x_index < 0 || x_index >= t->dim_x )
            continue;
        else if ( y_index < 0 || y_index >= t->dim_y )
            continue;
        else if ( z_dis >= t->body_height )
            continue;
       
        if ( z_dis < 0 )
        {
            if ( z_dis < -( t->roots_depth ) )
                continue;
            if ( (t->roots[z_dis][ x_index + y_index * t->dim_x ].whole & 0x7F) != 0 ) //any non-blocked tree_tile
                return p;
        }
        else if ( (t->body[ -1 - z_dis ][ x_index + y_index * t->dim_x ].whole & 0x7F) != 0 )
            return p;
    }
   
    return NULL;
}
I haven't tested it at all yet.

Is there an existing .cpp I should put it in, or should I create a new .h and .cpp for plant-related DF functions?

[PatrikLundell]

@Bumber:
The x and y negative tests can be simplified to "if (x < -47)", etc., although I don't understand why negative values should be accepted at all (and I think some indexing would blow up later if the index is negative).
Similarly, values greater-equal to the embark size bound will be rejected even if you truncate them back to the boundary (i.e. equal), so why not:
"if (x < 0 || y < 0 || x >= world->max.x_count || y >= world->map.y_count) return NULL;" (but formatted nicely, not an ugly one-liner)?

[lethosor]

Did the "x / 48 * 48 < 0"-style checks come directly from disassembly? I agree with PatrikLundell that there is a bounds issue there: -47 / 48 == 0, so some small negative numbers will slip through those checks.

I think the first section could be simplified (or corrected) to:

Code: [Select]

if (x < 0 || x >= world->map.x_count || y < 0 || y >= world->map.y_count)
  return NULL;
x_count and y_count do happen to be multiples of 48 currently, so the "/48 * 48" bits don't affect checks against those fields, but I suppose the behavior could be different if map sizes change in the future.

I think this could be a good fit for the Maps module (modules/Maps.cpp), by the way. Gui::getAnyPlant() could definitely stand to be optimized by using your logic, but I don't think yours really belongs in the Gui module.

[Bumber]

The x and y negative tests can be simplified to "if (x < -47)", etc., although I don't understand why negative values should be accepted at all (and I think some indexing would blow up later if the index is negative).

That would be caught by the checks on x_index and y_index, though there might be potential for an incorrect result if it falls in bounds somehow.

Did the "x / 48 * 48 < 0"-style checks come directly from disassembly?

Yes:

Code: [Select]

movsx   r13d, dx ; x_coord into r13
mov     eax, 2AAAAAABh ; (2^32 / 6) + 1 into rax {magic number to divide by 6}
mov     esi, r13d ; x_coord into rsi
imul    r13d ; x_coord / 6 into rdx
sar     edx, 3 ; (x_coord / 6) / 8 = x_coord / 48 into rdx
mov     eax, edx ; x_coord / 48 into rax
shr     eax, 1Fh ; sign bit of (x_coord / 48) into rax
add     edx, eax ; add 1 to (x_coord / 48) if negative {correct result for negative numbers}
lea     eax, [rdx+rdx*2] ; (x_coord / 48) * 3 into rax
movzx   edx, r13w ; x_coord into edx
shl     eax, 4 ; (x_coord / 48) * 3 * 16 = (x_coord / 48) * 48 into rax {x tile coord of MLT}
sub     esi, eax ; x_coord - ((x_coord / 48) * 48) = x_coord%48 into rsi
sub     dx, si ; x_coord - x_coord%48 = (x_coord / 48) * 48 into rdx {x tile coord of MLT (again)}
mov     [rsp+30h+arg_10], esi ; save x_coord%48
js      loc_7FF65FC211A2 ; if (x_coord - x_coord%48) negative, return null
movsx   eax, dx ; (x_coord / 48) * 48 into rax
cmp     eax, cs:_world.map.x_count
jge     loc_7FF65FC211A2 ; if ((x_coord / 48) * 48) >= map.x_count, return nullI've cut out the y_coord stuff that's mixed with the x_coord stuff for clarity. (Function arguments are actually supposed to be int16_t instead of int32_t. Don't think this makes a difference.)

I think I'll ask about it in the FotF thread.

[PatrikLundell]

:
x_count and y_count do happen to be multiples of 48 currently, so the "/48 * 48" bits don't affect checks against those fields, but I suppose the behavior could be different if map sizes change in the future.
:

All of that code is directly tied to the current organization of the map, and so would have to be reviewed at a minimum after the map rewrite (which probably would split the plants data into one vector per layer at a minimum, and possibly rework things completely), and probably rewritten from scratch to provide a new logic appropriate for the new logic organization. I'm fairly certain embark size granularity won't change before the map rewrite, unless the rewrite is postponed significantly for some reason.

[Clément]

Did the "x / 48 * 48 < 0"-style checks come directly from disassembly?

Yes:

Code: [Select]

...

This assembly is compiled from "x - x%48" not "(x/48)*48". It's funny because the compiler compute "x%48" from "x - (x/48)*48", so it ends up computing "x - (x - (x/48)*48)" and not seeing the useless double subtraction. (reproduced with compiler explorer, msvc only, gcc and clang don't do the double subtraction).

This does not change the issue with negative numbers. But forgetting that C integer division is rounded toward 0 and not toward negative infinity is a common error.

[Bumber]

Why do some functions in Maps.h use "extern" and others don't? E.g.:

Code: [Select]

extern DFHACK_EXPORT bool RemoveBlockEvent(uint32_t x, uint32_t y, uint32_t z, df::block_square_event * which );

DFHACK_EXPORT bool canWalkBetween(df::coord pos1, df::coord pos2);
How do I add my function to the Lua API? Seems like it's a bit more complicated than adding the line "WRAPM(Maps, getPlantAtCoords)," to LuaApi.cpp.
Edit: Trying to do it like "maps_ensureTileBlock" gives me the error "return value type does not match the function type".

Also, what am I looking at here: "WRAPN(getBlock, (df::map_block* (*)(int32_t,int32_t,int32_t))Maps::getBlock),"

[lethosor]

Why do some functions in Maps.h use "extern" and others don't? E.g.:

Code: [Select]

extern DFHACK_EXPORT bool RemoveBlockEvent(uint32_t x, uint32_t y, uint32_t z, df::block_square_event * which );

DFHACK_EXPORT bool canWalkBetween(df::coord pos1, df::coord pos2);
How do I add my function to the Lua API? Seems like it's a bit more complicated than adding the line "WRAPM(Maps, getPlantAtCoords)," to LuaApi.cpp.

Edit: Trying to do it like "maps_ensureTileBlock" gives me the error "return value type does not match the function type".

If you could provide the function signature and any relevant pieces of the error you're getting, that would help. WRAPM should be all that's necessary in most cases, but sometimes weird argument or return types can trip it up.

maps_ensureTileBlock() is an example of a custom-implemented function that uses the Lua C API (section 4 of https://www.lua.org/manual/5.3/manual.html). That is an option, but the WRAP macros are much less maintenance effort.

Also, what am I looking at here: "WRAPN(getBlock, (df::map_block* (*)(int32_t,int32_t,int32_t))Maps::getBlock),"

There are two implementations of Maps::getBlock - one takes three int32_ts and one takes a df::coord. Casting to a function pointer specifies which one to select. Without it, we'd get a fairly confusing error:

Spoiler (click to show/hide)

Code: [Select]

../library/LuaApi.cpp:1375:71: error: no matching function for call to ‘wrap_function(<unresolved overloaded function type>, bool)’
 1375 | #define WRAPN(name, function) { #name, df::wrap_function(function,true) }
      |                                                                       ^
../library/LuaApi.cpp:1854:5: note: in expansion of macro ‘WRAPN’
 1854 |     WRAPN(getBlock, Maps::getBlock),
      |     ^~~~~
In file included from ../library/LuaApi.cpp:40:
../library/include/DataFuncs.h:275:36: note: candidate: ‘template<class T> DFHack::function_identity_base* df::wrap_function(T, bool)’
  275 |     inline function_identity_base *wrap_function(T ptr, bool vararg = false) {
      |                                    ^~~~~~~~~~~~~
../library/include/DataFuncs.h:275:36: note:   template argument deduction/substitution failed:
../library/LuaApi.cpp:1375:71: note:   couldn’t deduce template parameter ‘T’
 1375 | #define WRAPN(name, function) { #name, df::wrap_function(function,true) }
      |                                                                       ^
../library/LuaApi.cpp:1854:5: note: in expansion of macro ‘WRAPN’
 1854 |     WRAPN(getBlock, Maps::getBlock),
      |     ^~~~~


On that note: thanks for taking the time to expose this to Lua!

[Bumber]

If you could provide the function signature and any relevant pieces of the error you're getting, that would help. WRAPM should be all that's necessary in most cases, but sometimes weird argument or return types can trip it up.

Code: [Select]

extern DFHACK_EXPORT df::plant *getPlantAtCoords(int32_t x, int32_t y, int32_t z);

inline df::plant *getPlantAtCoords(df::coord pos) { return getPlantAtCoords(pos.x, pos.y, pos.z); }
Was getting a red squiggly line for "return Lua::PushDFObject" in "static int maps_getPlantAtCoords(lua_State *L)" with the message. Error was result of copying from "maps_getTileBiomeRgn" and editing to "PushDFObject", instead of from "maps_ensureTileBlock" (which returns 1.)

Doing it like getBlock with WRAPN seems to work. Not used to working with function pointers, so the syntax was confusing.

How does the Lua C API way determine which implementation it's using?

Edit: Doesn't look like it's accepting DFCoord (using xyz2pos) in Lua using WRAPN with cast. C API works for both DFCoord and three int32_ts.

It's also crashing on tree branches, so I've got to debug that. I had the z indexes for "body" and "roots" swapped, causing it to always use a negative index.