Live View DataTable support

[narknon]

Builds using DTSupport branch on UEPseudo.

Do not merge until this (particularly the FName changes) have been tested properly, there's potential for big breaks.
Here are the FName changes for anyone curious: https://github.com/Re-UE4SS/UEPseudo/commit/0ecbab1afcf54ea607c0944854665b3c8f21464a

[UE4SS]

Both this and #554 add FMapProperty to UnrealDef.hpp.

[narknon]

Swapped to PR into staging branch as this may not be a 4.0 release given the amount of testing still to be done on tmap.

[narknon]

Both this and #554 add FMapProperty to UnrealDef.hpp.

Yes, this will rely on that but both need sufficient testing on their own so they need separate PRs.

[narknon]

@UE4SS Not sure if you're interested in helping finish this up. I think I'd be capable of it, but as you have dealt with structs in live view much more, it may be more efficient for you right now.

Essentially, the struct for the row should display the same as we do when it is a property (for now, maybe we add columns eventually), and edit should look the same as editing a struct property.

I think for adding a new row, we give a checkbox for "populate with data from previous row" that copies the buffer from the struct in the previous row into the edit buffer so that a user has a base to edit.

[UE4SS]

Perhaps you should leave some instructions for how to build this ? The PR description is completely blank.
I'm assuming I need to use one of the DT branches in the UEPseudo repo ? And I coincidentally noticed your DTSupport branch so I'm assuming that's the correct one and not the one that @bitonality has a PR based for.

[narknon]

Yes, I merged his PR into DT support branch

[UE4SS]

Build error: RE-UE4SS\deps\first\Unreal\src\UDataTable.cpp(10): fatal error C1083: Cannot open include file: 'Unreal/TMap.hpp': No such file or directory

[UE4SS]

I'm getting several build errors, with both DTSupport branches, and you can build this right now right ? Not sure what I'm doing wrong.

[narknon]

Sorry, didn't push the few updates to rebase Bit's PR. Pull now

[UE4SS]

It now builds successfully.
Will do some testing.

[narknon]

TMap required edits to FName that weren't fully tested, so I'm a little bit suspicious of that. But I figured implementing DT support in Live View would make testing tmap/FName alignment with the dynamic type lib easier, so I don't think the work here is premature.

[narknon]

Ok, initial problem is that below a certain version DT had way less virtuals. We will have to change add/remove to be our own methods. I had begun implementing that in another branch before deciding to merge Bit's over mine.

[UE4SS]

@narknon Someone's been using tabs instead of spaces in your UEPseudo branch.

[narknon]

@narknon Someone's been using tabs instead of spaces in your UEPseudo branch.

Yes, UE has. Will need to be cleaned up.

[UE4SS]

@narknon Someone's been using tabs instead of spaces in your UEPseudo branch.

Yes, UE has. Will need to be cleaned up.

And actually, it looks like the indentation is all messed up because even replacing \t with four spaces doesn't fix the indentation.

[narknon]

@narknon Someone's been using tabs instead of spaces in your UEPseudo branch.

Yes, UE has. Will need to be cleaned up.

And actually, it looks like the indentation is all messed up because even replacing \t with four spaces doesn't fix the indentation.

Yes, for most files this has been an issue for a while. I think some snuck in when I brought stuff in, though I was fixing most, and some from when CC brought files in.

[narknon]

Testing changes to datatable.hpp/cpp now.

[UE4SS]

Must you copy & paste so much ? It makes things a lot more difficult.
It would've been better if you looked at our existing code, understood it, and wrote code for datatables based on that knowledge.
Now we have a lot of random unrelated enum code mixed in with the datatable code.
That said, I don't know what the state of this PR is exactly.
Is it super early and you're planning on changing or rewriting a lot ? If so, I wouldn't even bother testing this or even looking at it right now, just let us know when it's ready to be reviewed and tested.

Anyway, the small amount of testing I got done before I noticed the state of the code resulted in a crash when trying to use the + button to presumably add a row.
It crashed when it tried calling the virtual in UDataTable::AddRow.

[UE4SS]

Our FTableRowBase struct is wrong.
As far as I can tell, it shouldn't have any member variables.

[UE4SS]

I think we should leverage our already existing struct editing code as much as possible with data table editing.
The values as far as I can tell are just structs backed by a reflected UScriptStruct which we should be able to feed to our existing rendering functions to both display the data as well as to allow editing of that data.

[narknon]

Yeah, that was the intention. I didn't realize Bit's code wasn't tested below 4.21 though, so the base DT functionality still needs more work than I assumed. The Live View logic was copy pasted because I did understand it (I wrote most of the enum logic) and I just needed an easy way to test our DT methods, which it is serving to do.

[UE4SS]

How about something like this for the layout ?
Ignore the repeated name inside each tree node, I forgot to remove that.
I didn't add any way to create or delete a new entry because I haven't looked into that yet, this was just something I did because I wasn't happy with your copy & paste job and I didn't see a good way to force it into our existing rendering functions.

[narknon]

That could work and would work for now, but I don't think that format makes it very easy to compare rows when viewing all (even if it had an expand all), which I think is important for this. I'm not at my PC right now to look at the existing logic to see how difficult it'd be to do what I'm picturing, so I can't say which way we should go right now. Is rendering structs currently an if statement in the property for loop? If so, maybe it should be pulled out to its own function? Idk, probably not any point in my hypothesizing until I look at it again.

[narknon]

Currently there is a crash when adding on lower versions. At least in 4.15, haven't checked the exact cutoff. Crash at icppstructs::copy.

[UE4SS]

Is rendering structs currently an if statement in the property for loop? If so, maybe it should be pulled out to its own function? Idk, probably not any point in my hypothesizing until I look at it again.

No, but it uses the currently selected object and we're not at all set up to select arbitrary structs, we can only really select a property or a UObject derivative.
I bypassed that annoyance by adding two params to render_properties which effectively override the selected object.
As a result, I think we can use render_properties(struct_data_ptr, ustruct_ptr) to render the structs anywhere on the current frame.

That could work and would work for now

Do you want me to push my changes to this PR ?

[narknon]

Do you want me to push my changes to this PR ?

Maybe to another branch off of this one. Part of the point of this branch is to be able to easily test add/remove of dt rows to test TMap and UDataTable generally, so can't really remove those right now.

[narknon]

Now getting crashes on Add in 4.27 as well for some reason.

In UScriptStruct::ICppStructOps::Copy

[UE4SS]

My branch (alternative UI) is available here: https://github.com/UE4SS-RE/RE-UE4SS/tree/dt-support-alternate-ui

[UE4SS]

I guess I should mention that my crash when adding was in UDataTable::AddRow in my 5.4 demo game.

[narknon]

I guess I should mention that my crash when adding was in UDataTable::AddRow in my 5.4 demo game.

We don't even have 5.04 memberoffsets/vtable offsets for UDataTable.

[UE4SS]

I guess I should mention that my crash when adding was in UDataTable::AddRow in my 5.4 demo game.

We don't even have 5.04 memberoffsets/vtable offsets for UDataTable.

That'd be why then 😆

[narknon]

Latest pushes on UEPseudo swap away from using vtables for add/remove anyway.

[narknon]

Though you'd need at least member var ofc

[narknon]

Our FTableRowBase struct is wrong. As far as I can tell, it shouldn't have any member variables.

I think Bit did that to store size and alignment to use when adding to the map, but I'm not sure that is actually required.

[narknon]

Casting to FTableRowBase does not work to allow CopyScriptStruct to get the correct size/alignment when copying into the destination struct. We can feed a pointer directly to AddRowInternal if we are confident in the creation of our row struct, and that works fine.

I think for LiveView add functionality I'll add a duplicate row method that uses add row internal and then just allows editing after that.

Not sure if I'm missing something for a way to cast into FTableRowBase and have it actually work correctly.

[bitonality]

Yeah, that was the intention. I didn't realize Bit's code wasn't tested below 4.21 though, so the base DT functionality still needs more work than I assumed. The Live View logic was copy pasted because I did understand it (I wrote most of the enum logic) and I just needed an easy way to test our DT methods, which it is serving to do.

This was the tradeoff of going the virtual route because we couldn't reimpl UDataTable (in the copy pasta fashion) because TMap didn't have a full reimpl in UEPseudo. Now that we have TMap, we could consider reimpl'ing (copy pasta) UDataTable as well, but that comes with its own set of issues.

I have no experience to share in terms of doing add/remove to DTs without using the virtuals since the scope of my contributions was to only leverage virtuals to maximize runtime stability for all versions that support the full set of DT virtuals. With virtuals, we can clearly error to the modder that "hey DataTables aren't safely implemented in your version, but here's a pointer to the RowMap if you want to try".

My opinion is that we could release a virtual-only DT impl (that work is done) for versions 4.21+ and then expose the GetRowMap() for ALL versions if modders want to try and make it work in their versions. Even if we can completely swap out the current virtual calls for the reimpl'ed TMap/DataTable, we'd have to compare the compatibility to see which DT API (virtual vs UEPseudo reimpl) has better compatibility coverage which is somewhat unfeasible at the moment unless someone wants to put in a ton of work manually testing.

Does anyone have any hunches on whether the UE Core reimpl TMap/DT or virtual-only DT would work across more game configurations? How does that factor in for deciding what APIs we want to support for what versions?

Sidebar...

The DynamicType work is somewhat orthogonal to this, but I think that needs integrated before we release any flavor of DT api.

Currently, both flavors of DT can break when someone tries to cast the actual row value into their dumped game struct.

struct CraftingRecipe {
    // This struct would be dumped by a modder from their game
}

// Within a mod...
uint8* dtRow; // This pointer could be from virtual FindRowUnchecked() or UEPseudo reimpled TMap.find()

// KABOOM if CraftingRecipe has FName members that don't each coincidentally align on eights.
CraftingRecipe * recipe = reinterpret_cast<CraftingRecipe *>(dtRow); 

// The following layout for CraftingRecipe would work in UE 4.11+ (and maybe even earlier)
struct CraftingRecipe {
    FName Item1;
    uint8 Quantity;
}

// This layout is broken for UE 4.22+
struct CraftingRecipe {
    uint8 Quantity;
    FName Item1;
}

[narknon]

Virtual only would work across very few versions. DT support isn't something I want to be half supported, it should be all versions. We could call the virtuals instead of running our own logic for versions that support them but by the time our own reimplementation is finished there shouldn't be a difference in stability.

[narknon]

@UE4SS how does our StaticStruct system work (if at all)? I know staticclass is implemented to an extent.

[UE4SS]

@UE4SS how does our StaticStruct system work (if at all)? I know staticclass is implemented to an extent.

I don't know what you mean by "StaticStruct" system, please expand.

[narknon]

Well, I guess it's easier to just ask for help looking into why ICppStructOps::Copy hangs after passing the data to a structs = operator. I can't figure it out, it doesn't always happen. My best guess is that it's related to our FTableRowBase or not operating on the table in the game thread or something.

It can easily be repro'd by using the + sign on a DT in live view.