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

Add a simplistic C API for all unsigned data type widths as cell values to allow language interop #42

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

KitsuneAlex
Copy link
Contributor

@KitsuneAlex KitsuneAlex commented Dec 9, 2024

This would allow your library to be used in other languages which cannot directly interact with C++ through a simplistic C interface.

If you decide to merge this, it should be merged after my last fix PR.

Thanks for your excellent work on this project :)

@KitsuneAlex
Copy link
Contributor Author

Lightly tested on Windows x64, Linux x64/arm64 and macOS x64/arm64. Also confirmed working with tools like jnih and Kotlin CInterop :)

@facontidavide
Copy link
Owner

Hi.

Thanks for taking this initiative!

My suggestions are:

  • please no change in the namespace ("boncai"). Keep it "bonxai"
  • no alternative subdirectory. Instead, create a file called bonxai_core/include/bonxai/bonxai.h and add a target to the existing bonxai_core/CMaleLists.txt
  • remove changesto "serialize"

@KitsuneAlex
Copy link
Contributor Author

Hi.

Thanks for taking this initiative!

My suggestions are:

  • please no change in the namespace ("boncai"). Keep it "bonxai"
  • no alternative subdirectory. Instead, create a file called bonxai_core/include/bonxai/bonxai.h and add a target to the existing bonxai_core/CMaleLists.txt
  • remove changesto "serialize"

Thanks for replying so quickly! Sounds good to me, will do :)

@KitsuneAlex
Copy link
Contributor Author

This should do

@KitsuneAlex
Copy link
Contributor Author

I commented on the other closed PR btw if you didn't get notified, the print was your original code, i removed it in an earlier PR which was a mistake from my end, that's why i added it back. Your serilization will likely break without it, which i didn't realize before.

Comment on lines +33 to +34
#define BONXAI_FALSE 0
#define BONXAI_TRUE 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As bool's and enum's are not exactly ideal for language interop scenarios because of unportable ABI an int type is usually employed, and if you have your boolean alias you might as well have fitting constants. These can be recognized by interop tools.

Comment on lines +84 to +87
sprintf(
header, "Bonxai::VoxelGrid<%s,%d,%d>(%lf)\n", type_name.c_str(), grid.innetBits(),
grid.leafBits(), grid.voxelSize());

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was your code. I removed it in my previous PR as i assumed it was a debug line, but it wasn't. If you don't add this code back your serialization will be broken i think.

BONXAI_DEFINE_GRID_IMPL(u8, uint8_t)
BONXAI_DEFINE_GRID_IMPL(u16, uint16_t)
BONXAI_DEFINE_GRID_IMPL(u32, uint32_t)
BONXAI_DEFINE_GRID_IMPL(u64, uint64_t)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean... since you are there, add int8, int16, int32, int64, float, double

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

#include <streambuf>

namespace Bonxai {
struct PointerStreamBuffer final : public std::streambuf {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add some doumentation. i don't know what the purpose of this is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These templates allow interacting with your i/ostream based serialization APIs through C-typical slices, that is a pair made of a pointer and a size since streams are not a concept in C itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

#include <vector>

namespace Bonxai {
class VectorStreamBuffer final : public std::streambuf {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can do but this is an implementation detail, not sure if it needs much documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

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.

2 participants