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

Enables images to be read and modified per pixel in shader #2331

Merged
merged 11 commits into from
Jan 8, 2025

Conversation

codex128
Copy link
Contributor

This PR enables images to be read/written in a shader using imageLoad, imageStore, etc. This is done by calling glBindImageTexture when an access flag is not null after the texture is bound with glBindTexture.

This feature is disabled by default. Unless the access flag is specifically set to not null on the image, the image will be bound normally.

@codex128
Copy link
Contributor Author

The compiler seems to think that LwjglGL doesn't override the abstract method I added to GL4 (it clearly does). Would a maintainer mind looking at this?

@zzuegg
Copy link
Member

zzuegg commented Nov 25, 2024

From a quick look to the files changes menu it seems you missed to add the overwrite to the lwjgl2 renderer.

@zzuegg
Copy link
Member

zzuegg commented Nov 25, 2024

I am not a big fan of the enum. There are 3 more possible flags available. ( coherent, restricted and volatile) and you are encouraged to use a combination. Like
"coherent | restricted | readOnly" is a valid flag.

Edit: got confused with glsl parameters. You can dismiss this comment

@codex128 codex128 marked this pull request as draft December 13, 2024 02:21
@codex128 codex128 marked this pull request as ready for review December 20, 2024 16:12
@codex128
Copy link
Contributor Author

codex128 commented Dec 20, 2024

I noticed some serious problems with my original approach about a week ago. Today I went and implemented a different approach which I believe is far more flexible, so this PR is now ready for review again.

New usage:

TextureImage img = new TextureImage(myTexture, TextureImage.Access.ReadWrite);
material.setParam("MyImage", VarType.Image2D, img);

This allows for a single texture to be bound differently in different situations, without needing to change values on the texture's image in each situation, which could easily lead to unexpected behavior.

@zzuegg
Copy link
Member

zzuegg commented Dec 20, 2024

I think it would be even saver to have the access flags defined in the j3md? kind of the same way we have the -linear option on textures. But your current solution looks better then the first one.

@yaRnMcDonuts yaRnMcDonuts added this to the v3.8.0 milestone Dec 24, 2024
@codex128
Copy link
Contributor Author

I experimented with having the j3md define the access flag, but I don't think it is worth it. The material and j3md loader code is a mess and not change-friendly at all. I feel like I'm fighting the code just to put the necessary changes in. I think this PR should be left as-is, because it's already very neat and doesn't change anything that could have far-reaching consequences. Plus, the access flag defaults to ReadWrite, which is slightly less efficient but otherwise fine for all situations.

@yaRnMcDonuts
Copy link
Member

yaRnMcDonuts commented Jan 5, 2025

@codex128 I'll go ahead and merge this prior to the next alpha release in a few days unless anyone contests before then.

The only review I have is that it could be useful to make a simple example or some code snippets to show how to use this new feature once it makes it to the stable release. But for now everything looks good and it sounds like this is ready to be merged.

@codex128
Copy link
Contributor Author

codex128 commented Jan 5, 2025

I have added a test app, but I ran into an error that made me unable to run the test.

run:
Jan 05, 2025 11:26:05 AM com.jme3.system.JmeDesktopSystem initialize
INFO: Running on jMonkeyEngine null-SNAPSHOT
 * Branch: pipelineApi
 * Git Hash: 986603c
 * Build Date: 2024-09-03
Jan 05, 2025 11:26:05 AM com.jme3.system.lwjgl.LwjglContext printContextInitInfo
INFO: LWJGL 3.3.3+5 context running on thread jME3 Main
 * Graphics Adapter: GLFW 3.4.0 Wayland X11 GLX Null EGL OSMesa monotonic shared
Jan 05, 2025 11:26:05 AM com.jme3.renderer.opengl.GLRenderer loadCapabilitiesCommon
INFO: OpenGL Renderer Information
 * Vendor: NVIDIA Corporation
 * Renderer: NVIDIA GeForce GTX 1060 6GB/PCIe/SSE2
 * OpenGL Version: 3.3.0 NVIDIA 560.35.03
 * GLSL Version: 3.30 NVIDIA via Cg compiler
 * Profile: Core
Jan 05, 2025 11:26:05 AM com.jme3.audio.openal.ALAudioRenderer initOpenAL
INFO: Audio Renderer Information
 * Device: OpenAL Soft
 * Vendor: OpenAL Community
 * Renderer: OpenAL Soft
 * Version: 1.1 ALSOFT 1.23.1
 * Supported channels: 64
 * ALC extensions: ALC_ENUMERATE_ALL_EXT ALC_ENUMERATION_EXT ALC_EXT_CAPTURE ALC_EXT_DEDICATED ALC_EXT_disconnect ALC_EXT_EFX ALC_EXT_thread_local_context ALC_SOFT_device_clock ALC_SOFT_HRTF ALC_SOFT_loopback ALC_SOFT_loopback_bformat ALC_SOFT_output_limiter ALC_SOFT_output_mode ALC_SOFT_pause_device ALC_SOFT_reopen_device
 * AL extensions: AL_EXT_ALAW AL_EXT_BFORMAT AL_EXT_DOUBLE AL_EXT_EXPONENT_DISTANCE AL_EXT_FLOAT32 AL_EXT_IMA4 AL_EXT_LINEAR_DISTANCE AL_EXT_MCFORMATS AL_EXT_MULAW AL_EXT_MULAW_BFORMAT AL_EXT_MULAW_MCFORMATS AL_EXT_OFFSET AL_EXT_source_distance_model AL_EXT_SOURCE_RADIUS AL_EXT_STATIC_BUFFER AL_EXT_STEREO_ANGLES AL_LOKI_quadriphonic AL_SOFT_bformat_ex AL_SOFTX_bformat_hoa AL_SOFT_block_alignment AL_SOFT_buffer_length_query AL_SOFT_callback_buffer AL_SOFTX_convolution_reverb AL_SOFT_deferred_updates AL_SOFT_direct_channels AL_SOFT_direct_channels_remix AL_SOFT_effect_target AL_SOFT_events AL_SOFT_gain_clamp_ex AL_SOFTX_hold_on_disconnect AL_SOFT_loop_points AL_SOFTX_map_buffer AL_SOFT_MSADPCM AL_SOFT_source_latency AL_SOFT_source_length AL_SOFT_source_resampler AL_SOFT_source_spatialize AL_SOFT_source_start_delay AL_SOFT_UHJ AL_SOFT_UHJ_ex
Jan 05, 2025 11:26:05 AM com.jme3.audio.openal.ALAudioRenderer initOpenAL
INFO: Audio effect extension version: 1.0
Jan 05, 2025 11:26:05 AM com.jme3.audio.openal.ALAudioRenderer initOpenAL
INFO: Audio max auxiliary sends: 2
Jan 05, 2025 11:26:05 AM com.jme3.app.LegacyApplication handleError
SEVERE: Uncaught exception thrown in Thread[jME3 Main,5,main]
java.lang.AbstractMethodError: Receiver class com.jme3.renderer.lwjgl.LwjglGL does not define or inherit an implementation of the resolved method 'abstract void glBindImageTexture(int, int, int, boolean, int, int, int)' of interface com.jme3.renderer.opengl.GL4.
	at com.jme3.texture.TextureImage.bindImage(TextureImage.java:142)
	at com.jme3.renderer.opengl.GLRenderer.setTextureImage(GLRenderer.java:2766)
	at com.jme3.material.Material.updateShaderMaterialParameter(Material.java:872)
	at com.jme3.material.Material.updateShaderMaterialParameters(Material.java:908)
	at com.jme3.material.Material.render(Material.java:1093)
	at com.jme3.renderer.RenderManager.renderGeometry(RenderManager.java:842)
	at com.jme3.renderer.RenderManager.renderGeometry(RenderManager.java:772)
	at com.jme3.renderer.queue.RenderQueue.renderGeometryList(RenderQueue.java:273)
	at com.jme3.renderer.queue.RenderQueue.renderQueue(RenderQueue.java:315)
	at com.jme3.renderer.RenderManager.renderViewPortQueues(RenderManager.java:1117)
	at com.jme3.renderer.RenderManager.flushQueue(RenderManager.java:1012)
	at com.jme3.renderer.pipeline.ForwardPipeline.pipelineRender(ForwardPipeline.java:117)
	at com.jme3.renderer.RenderManager.renderViewPort(RenderManager.java:1313)
	at com.jme3.renderer.RenderManager.render(RenderManager.java:1355)
	at com.jme3.app.SimpleApplication.update(SimpleApplication.java:283)
	at com.jme3.system.lwjgl.LwjglWindow.runLoop(LwjglWindow.java:631)
	at com.jme3.system.lwjgl.LwjglWindow.run(LwjglWindow.java:721)
	at java.base/java.lang.Thread.run(Thread.java:833)

I think this is probably a problem in my development environment.

@codex128 codex128 requested a review from capdevon January 6, 2025 17:30
Copy link
Contributor

@capdevon capdevon left a comment

Choose a reason for hiding this comment

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

LGTM

@codex128
Copy link
Contributor Author

codex128 commented Jan 7, 2025

Thank you for the reviews, @capdevon and @zzuegg! If no one has anything else to add, this PR is ready to be merged.

@yaRnMcDonuts yaRnMcDonuts merged commit 662b4ea into jMonkeyEngine:master Jan 8, 2025
15 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.

4 participants