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

1st, 3rd, 4th dimension #5

Merged
merged 10 commits into from
Oct 3, 2023
Merged

1st, 3rd, 4th dimension #5

merged 10 commits into from
Oct 3, 2023

Conversation

GDBobby
Copy link
Collaborator

@GDBobby GDBobby commented Sep 30, 2023

Pull Request

1st dimension wasn't added.
The performance difference between a 1st dimension section and a 2nd dimension with only 1 depth in the y dimension should be negligible.

3rd dimension

rebased from a separate branch
point cloud example moved from a separate branch.
Point Cloud example builds a obj file

4th dimension

4th dimension added

no viable 4th dimension example is currently known. I believe the limiting factor is that we either need a custom file format to export the data, or we need a 3d rendering framework/engine to directly render the data without worrying about transferring between programs.

finished i think? probably some bugs lingering
I need to add an example
surprised the github test didnt find it
Comment on lines 56 to 75
#if 0 //internal testing
noiz::Noise2f noise2;
noiz::Noise_Processor2<float> noise_processor2{noise2};

noiz::Vec3f testing_point;
noise_processor.basic_processing(testing_point);
noise_processor.billowy_processing(testing_point);
noise_processor.hybrid_multi_fractal_processing(testing_point);
noise_processor.rigid_processing(testing_point);
noise_processor.turbulence_processing(testing_point);
noise_processor.raw_noise(testing_point);

noiz::Vec2f testing_point2;
noise_processor2.basic_processing(testing_point2);
noise_processor2.billowy_processing(testing_point2);
noise_processor2.hybrid_multi_fractal_processing(testing_point2);
noise_processor2.rigid_processing(testing_point2);
noise_processor2.turbulence_processing(testing_point2);
noise_processor2.raw_noise(testing_point2);
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Commented code, should remove before merging.

Comment on lines 15 to 23
Type left_top_below{};
Type right_top_below{};
Type left_bottom_below{};
Type right_bottom_below{};

Type left_top_above{};
Type right_top_above{};
Type left_bottom_above{};
Type right_bottom_above{};
Copy link
Member

Choose a reason for hiding this comment

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

I don't really have the visualization in my head like you do, but shouldn't the suffixes be front / back or something? Since below is basically = bottom.

///
/// \brief Obtain the next random unit Vec.
/// \returns The next random unit Vec.
///
/// //i dont think this is used anywhere
Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's not used by the library itself, but is available for users.

Copy link
Member

Choose a reason for hiding this comment

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

(Remove the comment though.)

Copy link
Member

Choose a reason for hiding this comment

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

Nice!

Copy link
Member

Choose a reason for hiding this comment

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

Oof lol

//if this is multi-threaded, accessing these variables directly could lead to strange behavior
//i.e. a handful of threads processing noise, then another comes in and changes octave??
//not sure if thats a use case worth covering?
noiz::Noise2<Type>& noise;
Copy link
Member

Choose a reason for hiding this comment

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

Storing a reference member deletes assignment, which is OK if this class isn't intended to be stored / used as a member itself. But if it is, then it will be annoying to deal with:

auto np = NoiseProcessor2{};
np = NoiseProcessor2{}; // compile error

And the same error will cascade through any types that have it as a member.

To avoid that, use std::reference_wrapper<Noise2<Type>> or store a pointer (but that will then require null checks everywhere, so it's worse).

Comment on lines 30 to 33
auto raw_noise(noiz::Vec2<Type> const& point) -> Type{
//redundant
return noise.at(point * step);
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this function could be const?

Copy link
Member

Choose a reason for hiding this comment

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

Lots of comments here, not sure if you want to commit all of them. File is not fully formatted either.

In general you want to keep member functions of a class const unless they change visible state of the type (ie users can notice the change). Since Noise_Processor2 stores a reference (wrapper) and exposes a bunch of knobs and tweaks, and most of its member functions don't change any of that state (ie don't rebind the reference to another Noise2 instance, and don't modify the knobs), all of them should be const.

//16 is 2^4
std::array<Type, 16> corners; //NOLINT
//16 is 2^4
std::array<Type, 16> corners;
Copy link
Member

Choose a reason for hiding this comment

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

Should initialize this with {}, would risk being garbage otherwise.

### grid4

was missing a few )
wasnt calling index.positions.at(), but instead calling index.at(), now fixed

An example for noise4 would expose similar bugs laying around

### Processor

processor2/3/4 abstracted to Processor
changes in noise2/3/4 and vec2/3/4 to reflect the processor abstraction
PointCloud example updated to include new usage
clang pls
Copy link
Member

@karnkaul karnkaul left a comment

Choose a reason for hiding this comment

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

Nice!

@GDBobby GDBobby merged commit 6c9164f into main Oct 3, 2023
2 checks passed
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