search

4bb4 / implot-rs

Rust bindings to https://github.com/epezent/implot
Apache License 2.0
54 stars 23 forks source link

Manual edit bindings #16

Closed a1ien closed 3 years ago

a1ien commented 3 years ago

Use imgui type from imgui-sys

This allow use helper conversion from ImGui-sys crate. For example https://github.com/imgui-rs/imgui-rs/blob/955bbf214e8f1df4ba21e95a1557a59d16199c27/imgui-sys/src/lib.rs#L46 And prevent clashing. Also imgui-sys generate better binding.

benmkw commented 3 years ago

I think this needs to be automated such that it can be more maintainable, something like this https://github.com/4bb4/implot-rs/blob/bfcaad3ad1d439c26a20c3b2b3441cbe1143af19/implot-sys-bindgen/src/main.rs#L37-L38 (from https://docs.rs/bindgen/0.57.0/bindgen/struct.Builder.html#method.whitelist_type) could maybe be done to include everything starting with ImPlot but excluding the ones which only start with Im. If such a pattern is not sufficient I think it could be an option to manually specify the names which is still easier than manually editing the generated bindings.

a1ien commented 3 years ago

Ok. I try change implot-sys-bindgen/src/main.rs

a1ien commented 3 years ago

Update. We still need edit file manual because bindgen remove #[derive(Debug, Copy, Clone)] from ImPlotStyle and ImPlotInputMap. And we must remove ImPlot_AnnotateVStr, ImPlot_AnnotateVVec4, ImPlot_AnnotateClampedVStr, ImPlot_AnnotateClampedVVec4 because we can't create correct __va_list_tag

4bb4 commented 3 years ago

I like the idea of #17, the lengthy code to set limits has been bothering me as well. I also like the From implementations you add in #16, though I really want to automate the bindings creation. I'd be OK with a little post-processing of the generated bindings so long as it's automatic. If I read https://github.com/4bb4/implot-rs/pull/16#issuecomment-795323441 right, the issues with the current bindings output are:

This should be doable by some (automatic) post-processing of the bindings and maybe https://docs.rs/bindgen/0.57.0/bindgen/struct.Builder.html#method.derive_copy and https://docs.rs/bindgen/0.57.0/bindgen/struct.Builder.html#method.derive_debug? I've not tried that yet so I don't know if it'd lead to other issues.

a1ien commented 3 years ago

derive_copy and derive_debug enable by default. This not work here because ImPlotStyle and ImPlotInputMap use ImVec2 from imgui and bindgen can't see what this type implement. When we call implot-sys-bindgen and must only commit new changes. This can be easily done with the git gui just stage necessary changes.

4bb4 commented 3 years ago

I tried your changes from https://github.com/4bb4/implot-rs/pull/17 and added a commit on top that fixes things to the point of them building and running, without using any of the changes here, in branch https://github.com/4bb4/implot-rs/tree/improve_api_experiments. One can now replace ranges with [x_min, x_max].into() and things work as you proposed in https://github.com/4bb4/implot-rs/issues/15, at least for limit setting.

This probably doesn't fix all use cases yet, like you list above, things that take ImVec2 as input may require a different approach, but then imgui-rs should implement those conversions, not implot-rs. I also don't really agree with just re-exporting everything from imgui-sys - imgui should be imported by the user separately and used through that import.

a1ien commented 3 years ago

Do you try run implot-sys-bindgen whit this changes? It's very easy to manual edit after update to new cimplot version. And also diverse from imgu-rs. We must use ImVec2 and other from implot-rs instead generate our self binding.

4bb4 commented 3 years ago

It took me a while to understand what you mean, but I think I understand now. What you are saying is that the imgui types such as ImVec2 should really be used from the imgui_sys crate and not re-created from the C code via the implot bindings. That makes sense.

I've added the changes from the PR here in https://github.com/4bb4/implot-rs/commit/5779f7e6d67b681ca309de72593a9b967322455c and documented them a bit more, so no manual editing is needed but the effect is the same. We can hence close the PR here. I'll change signatures a bit more in the future so we don't even need to write &[ 0.0, 1.0].into() anymore in the future but instead can just write [0.0, 1.0] in places like https://github.com/4bb4/implot-rs/blob/f2a4c6a3d8919ec3438631873ce6a9f94135089c/implot-examples/examples-shared/src/line_plots.rs#L342.

Thanks for your contribution and ideas!