Closed Dennis-van-Gils closed 1 year ago
endolith
commented
2 years ago There are visible seams in your images :/ demo_looping_animated_2D_image.gif has four visible quadrants, for instance.
Note that if you're sampling on a rectangular grid, another way to make seamlessly-looping noise (in space or time) is to generate white noise and then lowpass filter it with an FFT or wrapping filter. I used it to make these tiles, for instance:
and these looping animations:
Dennis-van-Gils
commented
2 years ago @endolith Thanks for pointing out the seams as I did not spot them myself. I have some homework to do to find out why these quadrants appear.
Consider the pull request on hold for the moment.
Dennis-van-Gils
commented
2 years ago All is perfectly fine :) As it turns out, there are no seams present in the generated data arrays themselves. It is merely an artifact of an incorrect image export, luckily. I'll work on saving higher quality images to disk.... Be back soon with better images.
Dennis-van-Gils
commented
2 years ago Demo images are of higher quality now. No more seams. Ready for merge.
lmas
commented
2 years ago Hiya Dennis, sorry for the late reply. I loved the animated output and your use case sounds great! (Off-topic, but I would like to follow your project)
I did a quick review of your PR and am working on putting down notes for it now but need more time with it, I just wanted to let you know that I've seen it.
lmas
commented
2 years ago Yeah late again sorry, had to focus on course work and exams. And I had to spend some more time to think about how I really want to handle things with this library.
I love the work you did and would like to keep your code, but instead as a self-contained example in the examples dir.
My reasoning is that I've had pretty limited time to support the library since I started uni (which obviously can be seen in my late replies for the latest issues/PRs), so I changed my mind and don't want add new features (especially not without new tests) to the core internals unless there's a bigger demand for it and I don't want to introduce more code that utilises Numba (had poor experiences with it last year, hence the tiny note in my README that it's unsupported). I would rather want to let this library stay stable, but with limited features, for now and will try to put some more emphasis on this in the README.
Anyway, your thoughts? Is this a reasonable compromise?
Dennis-van-Gils
commented
2 years ago Hi Alex,
No worries about the long delay in answering. I understand that maintaining this library is not your day job.
Speaking from personal experience, maintaining a Python library that is in use by others over the world can be challenging. Especially when new features are requested or a pull request (PR) is being made that you as maintainer of the code are not very familiar with.
So I had the following in mind when I made my PR:
1) The new functions of the PR should not alter the existing code in any way.
2) The new functions should not enforce the use of Numba. Hence, it is fully optional. Though, for real number crunching as is easily the case for this PR, I would highly recommend using Numba as it can result in a speed gain of a factor of ~200.
3) The API should be as intuitive as possible.
4) Part of your existing code is flagged as "for backwards compatibility and might disappear in the future". It concerns the OpenSimplex class. This PR does not make use of any of that code.
I believe I have fullfilled all points above. This PR merely extends the existing library without affecting the current code or requirements. I hope I have mitigated your worries about stability as you write "I would rather want to let this library stay stable".
As you can tell, I would like to make my case to keep this PR. Besides the minimal intrusion as stated above, there is another point that complicates transforming my PR into a set of stand-alone examples as you suggest. The current API has the function _noise4() set as private (though private is a loose concept in Python). You currently provide a way to get access to this function by the public method noise4() of the OpenSimplex class, but this is flagged as "might disappear in the future". Hence, to keep this PR future proof and to prevent linter warnings on the improper use of 'private' functions, the new functions are best to be an integral part of this library.
I can add in automated test functions for the new functions of this PR if you wish. I should have done this from the get-go.
Note on Numba: I am a very happy user of Numba and use it in my scientific research for several years already. You write that you had issues in the past with Numba, but I think you have them all ironed out now. Your Numba code looks solid and performs well. Please let your past experience not prevent you from continuining with Numba. The speed benefits are tremendous. I am happy to help with further support on Numba if you need it, but again, I think your current Numba code is just fine now.
Thanks for taking your time of reading through my lengthy reply :) It is your call obviously and no offense will be taken if you do not want to merge this PR with or without alterations. Though I might turn my PR into a fork on PyPi (giving many credits to you, for sure) because I need my 'clients' to be able to download this new functionality via an easy install script.
Best regards and enjoy your studies, Dennis
lmas
commented
2 years ago Thanks for your reply Dennis, I like how you put care and effort into your work.
Obviously you couldn't know about my limited time (I'm swamped until christmas and have extremely limited time right now) and stance on contributions, but I like to encourage nice people so I'm willing to accept the PR if you're ready to do that final 10% polishing:
njit decorator since I'm already using it).These are my main complaints. I also think it's better to extend the class with your new public funcs (see bellow), but I want to do some more thinking and discuss that later if you're still willing to continue.
Following is some random comments.
Especially when new features are requested or a pull request (PR) is being made that you as maintainer of the code are not very familiar with.
Yeah, and most often it's better to ask and discuss, beforehand, with the maintainers before sending a PR with new features. I probably sound cheeky, but I think that most of us OSS developers really appreciate that and it saves time for everyone. :)
The new functions should not enforce the use of Numba. Hence, it is fully optional.
Appreciate it, but it's still adding in Numba dependent code regardless. My biggest issue with Numba was that I couldn't install it properly on FreeBSD (which I run on my systems) because of some 3rd party bug from llvm crashing the whole install. I've already spent too many hours trying to fix it locally and don't want to waste any more, so if I can't run it locally then I really can't and don't want to maintain any code using Numba extensively. That's final.
The API should be as intuitive as possible.
Having looping_animated_2D_image and friends accepting a seed and then using _init _noise4 doesn't feel very intuitive,
despite what you said about linter warnings and my deprecation note for the class.
It's better to extend the class and provide a public shortcut func, so it looks and behaves like everything else.
Also makes everything easier for, when ever or not, I finally decide to drop the class and rework the internals.
This PR merely extends the existing library without affecting the current code or requirements
Thank you for consideration, really appreciate it!
As you can tell, I would like to make my case to keep this PR.
I understand, you spent time working on it, which shows, and you claim to have a demand of this feature for your "clients", which I guess (I'm being nice) sorta meets my previously mentioned "usage demand" condition. :)
lmas
commented
1 year ago Closing due to inactivity, but feel free to reopen it in the future. Happy holidays.
Dennis-van-Gils
commented
1 year ago It took me some time to get around rethinking this PR. Your last post contains valid remarks. Indeed, I went a bit overboard by trying to push my PR without consulting you first. A case of being to over-enthusiastic ;)
I believe I have found a more elegant solution by simply turning the PR into a separate package that runs on top of your OpenSimplex library. I have called it opensimplex-loops. Our viewpoints still differ on the use of numba and numba-progress, and on the seed being passed as a parameter, and that's okay. This way, everyone can be happy.
Keep up the good work and cheers!
Hi @lmas. I found your OpenSimplex Python library and am very pleased by it. Great work. I intend to use your library to drive a turbulence generator for a scientific water tunnel I am designing. The coherent nature of Simplex noise is ideal here. The noise pattern we need should loop seamlessly over time.
Hence, I have taken the liberty to extend your library with several higher-level functions for the generation of seamlessly-looping animated images and closed curves, and seamlessly-tileable images. I believe that these functions are a very basic toolset for OpenSimplex noise generation and will likely be very useful to others as well.
I'll just point you to the examples folder and the following images to show you what the new functions do. Keeps this message still somewhat short.
All functions are fully optimized for numba and can show a progress bar during the noise generation which I find very helpful. I tested that the functions work both with numba and numba-progress in the environment, and without.
Please let me know if I can be of any help. Cheers.