Skip to content

Conversation

starcatter
Copy link

For comparison purposes with #110

The sample follows general steps (with multiple optimizations) described in the publication: Realtime GPGPU FFT ocean water simulation

Main focus of the sample is to demonstrate how to share compute/render resources between OpenCL and OpenGL to simulate an ocean surface.

@CLAassistant
Copy link

CLAassistant commented Dec 13, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ starcatter
❌ Piotr-Plebanski-Mobica
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

This is great work and I think we should try to get it and #110 in for the next release.

This PR seems to share a lot of files (e.g. OpenCL kernels) with #110. Should they be renamed so they are distinct? Or, is the intent that both samples will share the same OpenCL kernels?

}


static const char* IGetErrorString(int clErrorCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems generally useful. Could we move this to a shared location? Or, could we use the cl_util_print_error function already in the SDK utils lib?

Copy link
Author

Choose a reason for hiding this comment

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

Done.


int main(int argc, char* argv[])
{
OceanApplication app;
Copy link
Contributor

Choose a reason for hiding this comment

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

Because app is instantiated before parsing command line options, it is ignoring any passed-in command line options to select the platform or device index. Could we instead parse the command line options first and pass the chosen platform index, device index, and device type when we construct app? See the nbody sample as an example.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

This still didn't work for me, at least for the platform and device arguments (specified with "-p"), and it looks like the sample is exclusively running on platform zero. With my current config, this just so happens to be a CPU platform, so the sample isn't working. I'd really like to be able to select the platform to run with. Alternatively, we could try to find the right platform based on the OpenGL context with CL-GL sharing is enabled, but this isn't a viable solution when CL-GL sharing is disabled.

@bashbaug
Copy link
Contributor

bashbaug commented Apr 1, 2025

Hi @starcatter, thanks again for your contribution. Do you think you will have a chance to address the review comments above? Thanks!

@starcatter
Copy link
Author

@bashbaug Hello, sorry if I was a bit unresponsive! Those are good suggestions, I'll try to get them implemented this week.

@starcatter
Copy link
Author

This is great work and I think we should try to get it and #110 in for the next release.

This PR seems to share a lot of files (e.g. OpenCL kernels) with #110. Should they be renamed so they are distinct? Or, is the intent that both samples will share the same OpenCL kernels?

Yes, the intent was to use the same kernels in both samples. Only the shaders have subtle differences.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Couple other things will need to be fixed before we can merge:

  1. Can you please check that the CLA is all squared away? It's possible some of the latest commits were made with the wrong GitHub ID.
  2. Can you please take a look at the CI build errors? I didn't see them on my system, but we'll need to fix them one way or the other before merging.

VERSION 300
SOURCES main.cpp ocean.cpp ocean.hpp ocean_util.hpp
KERNELS twiddle.cl time_spectrum.cl inversion.cl normals.cl fft_kernel.cl init_spectrum.cl
SHADERS ocean.vert.glsl ocean.frag.glsl)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't able to build on my system without adding a glm dependency also:

Suggested change
SHADERS ocean.vert.glsl ocean.frag.glsl)
SHADERS ocean.vert.glsl ocean.frag.glsl
LIBS glm::glm)

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.

4 participants