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

[2]Implemented bencode serialization for Node and Octree #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

davids91
Copy link

Since I'd like to have the values stored, and have it as effective as possible I implemented some traits.

As an improvement in the future Nodes at certain LOD-s could be handled separately, so on-demand load could be possible. The current implementation makes it possible; but it only loads/saves the root node currently

@davids91 davids91 changed the title Implemented bencode serialization for Node and Octree [2]Implemented bencode serialization for Node and Octree May 4, 2023
Copy link
Owner

@Adam-Gleave Adam-Gleave left a comment

Choose a reason for hiding this comment

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

Hello!

Thanks for this PR, definitely agree that serialization would be nice to have, and pretty necessary towards publishing the crate anywhere.

However, I'm a bit wary about using this method, it seems pretty esoteric. I think a nicer route would be to just use the standard serde traits behind a feature gate, and then let the downstream user decide on a format (JSON/bincode/etc).

@davids91
Copy link
Author

davids91 commented May 17, 2023

That's a good thought, but for Voxel octrees there's also GVOX which provides more convenient voxel features.

For me this worked, but I ran into some performance issues, so Currently iterating on that. I actually Found it through serde; So targeting it sounds like a good plan

@davids91
Copy link
Author

Another thing worth considering is that lazy-loading should be possible with this!

@davids91
Copy link
Author

I separated the actual serialization methods here: https://github.com/davids91/svo-rs/blob/840ae2e883ade26e2a0a91cf9bb2cf04d31ead9e/src/node.rs#L392

I think that is something easily aligned with serde!
Might I suggest that this itself be pushed behind a feature, only if temporarily?

@davids91
Copy link
Author

davids91 commented Aug 17, 2023

Hey @Adam-Gleave I hope you are well! After implementing what you suggested in a separate repo: https://github.com/davids91/shocovox

I have come to the conclusion that performance-wise it would be better to use bendy itself, instead of ( or at least next to ) serde. The reasoning behind this is quite simple: as serde has a generic use-case target, I find it needs to write out the names of the struct fields it serializes.

The implementation provided here does not do that, it uses an implicit, order based identification of the struct fields (i.e.: the implementation is so that the field size is the first to be encoded, then follows min_position etc..). Because of this the process handles much less data.

So let's compare apples with oranges! The other implementation using serde has the following performance data for a size 32 octree:
init: 8.40325797 ms
write: 91.6077816 ms
read: 42.742902750000006 ms
query: 2.91500421 ms

The exact same things with my fork of your implementation:
init: 5.4165747799999995 ms
write: 25.533011939999998 ms
read: 7.66889124 ms
query: 1.7162132100000005 ms

Still the direction is yours to decide, I jut thought this would be relevant information!

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