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

Fixed iterators, added more tests, reorganized glium.rs, formating done. #4

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

Conversation

igorbologovv
Copy link

glium:

  • shader implementation was moved into it's own function
  • declaration of the render context was structured and implemented separately
    After changes in glium, state: selected was fixed, when rendering somehow;

iter.rs:
-fixed unit test test_bounds and renamed into test_bounds_quadtree
-added unit test for octree named test_bounds_octree()
-parameter depth was changed from u64 to u8 in all files(2.pow(256) is a bit too much);

coords.rs:

  • was implemented PartialOrd for OctVec and QuadVec too, particularly comparison of the depth is important;

tree.rs:
implemented with_capacity for initing preallocated memory for chunks and nodes;

lib.rs:
all doctests are fixed, warnings are fixed;

@Dimev
Copy link
Owner

Dimev commented Feb 14, 2023

Nice!
I'll take a better look in the weekend, looks good from a quick glance.

Copy link
Owner

@Dimev Dimev left a comment

Choose a reason for hiding this comment

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

What I found on first glance (will try running all changes later.

Not sure if I agree with making ChunkContainer public API.

Thanks for the extra tests!


/// A Lod Vector for use in a quadtree.
/// It subdivides into 4 children of equal size.
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Default, Debug, Hash)]
//#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Default, Debug, Hash)]
Copy link
Owner

Choose a reason for hiding this comment

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

This comment probably doesn't need to be here anymore

// get the lowest lod level
let level = self.depth.min(min.depth.min(max.depth));

// bring all coords to the lowest level
let self_difference = self.depth - level;
let min_difference = min.depth - level;
let max_difference = max.depth - level;

// println!("diff {:?}, {:?}, {:?}", self_difference, min_difference,max_difference);
Copy link
Owner

Choose a reason for hiding this comment

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

This comment too

@@ -149,13 +173,13 @@ impl LodVec for QuadVec {

let max_x = max.x >> max_difference;
let max_y = max.y >> max_difference;

// dbg!(min_x, min_y, max_x, max_y);
Copy link
Owner

Choose a reason for hiding this comment

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

This one too

) -> ChunksInBoundIter<L> {
debug_assert!(
Copy link
Owner

Choose a reason for hiding this comment

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

Not sure if I like this.

For me it would make more sense that it simply returns no chunks if the bounds are empty

src/iter.rs Outdated
}

impl SafeRngRange for rand::rngs::ThreadRng {
//#[no_panic]
Copy link
Owner

Choose a reason for hiding this comment

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

Remove this comment

src/iter.rs Outdated
((b.x - a.x) + 1) * ((b.y - a.y) + 1) * ((b.z - a.z) + 1)
}
///The same unit test as test_bounds juts for OctVec:
/// //todo check it once again
Copy link
Owner

Choose a reason for hiding this comment

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

This one too, and add a space for the doc comment

src/iter.rs Outdated
struct Chunk {
visible: bool,
cache_state: i32,
// 0 is new, 1 is merged, 2 is cached, 3 is both
Copy link
Owner

Choose a reason for hiding this comment

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

These can also be removed

src/iter.rs Outdated
4 => (position.x as i32 - r).pow(2) + (position.y as i32 - r).pow(2) < r,
_ => false,
};
// dbg!(position);
Copy link
Owner

Choose a reason for hiding this comment

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

These too

@@ -85,14 +88,15 @@ pub trait LodVec: std::hash::Hash + Eq + Sized + Copy + Clone + Send + Sync + De
/// let max_y = max.y >> max_difference;
///
/// // then check if we are inside the AABB
/// self.depth as u64 <= max_depth
/// self.depth <= max_depth
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the extra space

src/tree.rs Outdated
pub(crate) chunk: C,
pub(crate) index: usize,
pub(crate) position: L,
pub struct ChunkContainer<C: Sized, L: LodVec> {
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't intended to be public API IMO

@@ -1,3 +1,4 @@
//#![feature(generic_const_exprs)]
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefere this to not need nightly

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.

3 participants