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

Node Editor click and drag object selection not writing all ids to the provided IDLIntArray #13

Open
bploeckelman opened this issue Dec 2, 2024 · 5 comments

Comments

@bploeckelman
Copy link

I've been working on a game editor recently based on the imgui-node-editor blueprint example in the original library. Since the c++ blueprint code doesn't drop easily into another project as a usable starting point, I've been reworking it (using spaiR/imgui-java for the java bindings until about a week ago). Since it's in a reasonably workable (if incomplete) state now, I forked this repo and started porting over my java implementation in case you might want to use it.

I've run into a couple snags though, one of which is proving tough to sort out... Here's a link to the relevant snippet and comment from my fork

The gist is that node selection works one node at a time, but when trying to select multiple nodes via a click and drag rectangle the IDLIntArray holding the selected ids doesn't seem to be populating correctly on the native side, or rather the id values are correct on the native side (when set in imgui_node_editor_api.cpp in the BuildIdList(...) function) but only the first id is appearing on the java side after a call to NodeEditor.GetSelectedNodes(nodeIds, size) and all others are 0 even though the array size is correct. Ultimately that seems to be causing a native crash later on in a seemingly unrelated spot.

I've tried a handful of different ways to setup and use those arrays in case it was just a usage mistake on my part but nothing seems to have made a difference. The version linked above is about as close to the original blueprint example as possible.

Happy to provide more info, test additional things in java or native code, or walk through my blueprint code, just let me know what's helpful! Thanks!

@xpenatan
Copy link
Owner

xpenatan commented Dec 3, 2024

Hey.

Thanks for providing a way to test it. I'll take a look when I have some free time.

@xpenatan
Copy link
Owner

xpenatan commented Dec 5, 2024

Debugging in visual studio it seems there is a problem with resize. Need to investigate more.

@bploeckelman
Copy link
Author

Looking at the resize implementation in IDLHelper.h, it seems like the pattern used in the imgui-node-editor blueprint example shouldn't work in gdx-imgui since IDLArray.resize() sets all values to 0 with clear(). Though in my tests element [0] still had a valid (non-zero) value after the second selectedNodes.resize(numSelectedNodes) call in EditorSession.updateSelections().

Setting that aside, I removed all resize() calls and instead allocated selectedNodes with room for 10 elements. With that change it looks like the native BuildIdList(...) isn't writing the ids to the correct locations in the array. After click-drag selecting two nodes, both node ids (1 and 5) are in the array, just not in the right slots:

  • selectedNodes: {1, 0, 5, 0, 0, ...(remaining are zero) }

Since BuildIdList() on the native side takes its list param as an array of type SafePointerType<ax::NodeEditor::NodeId> and not plain int, could there be some padding that's not being accounted for due to the id type that's being written on the native side? SafePointerType is defined in imgui_node_editor.h but this is right at the edge of my understanding of memory layout and c++ template usage, so I don't have any other theories about why it looks this way.

While it was clunky to sort out, I didn't run into this issue with spaiR/imgui-java bindings, but there's also some type juggling cleverness being done there which is different than the IDL stuff here, such that users are able to pass primitive arrays to the equivalent functions:

Let me know if there's something else that it'd be helpful for me to check. Thanks!

@xpenatan
Copy link
Owner

xpenatan commented Dec 10, 2024

Hey, thanks for the help. I think it's working now. When you mentioned there was some sort of padding, I started looking at the NodeId type and realized it's a long, not an int. I updated all library methods to long.

Example using teavm.
image

It appears there's a bug on the TeaVM side. I selected three nodes, but only two were highlighted in the left panel. Since TeaVM's JsBody doesn't support long, jParser converts all native methods from long to int. This might be related to the issue or its something else.

@bploeckelman
Copy link
Author

This is great, thanks! I'm updating my fork today to continue working on this and ran into a snag. b3c501c updates int -> long in a bunch of places but I'm not sure what to do about BeginNode(), BeginPin(), Link(), Flow() and a handful of other methods in NodeEditor.

These are updated in nodeeditor.idl but NodeEditorCustom.h still shows them as taking int. So when I update the EditorObject id types to long in my fork, calls to NodeEditor.BeginNode(globalId) seem like they'd require a cast. Related to that, the pattern for drawing a Node or Pin is NodeEditor.begin(nodeId); ImGui.PushID(nodeId); ... and the ImGui.PushID() function still takes int without an override for long.

I could cast in all these cases but I want to clarify what the expected usage is before I go down that path, and also double check that I didn't miss something in rebuilding the natives / jni pieces. In case its helpful, these are the build steps I'm using once I updated my fork and rebased the node-editor-updates branch (the build instructions in the current readme didn't quite work when I was first getting this set up).

Thanks again for your help!

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

No branches or pull requests

2 participants