Skip to content

Conversation

@yaRnMcDonuts
Copy link
Member

@yaRnMcDonuts yaRnMcDonuts commented Mar 14, 2021

I've added all the shader files necessary for "AfflictedPBRTerrain.j3md" as well as "AfflictedAdvancedPBRTerrain.j3md" and I have also updated the paths to the vert/frag/glslib files accordingly.

@stephengold
Copy link
Member

Would it be possible to remove "Afflicted" from the filenames and add an example/testcase?

@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Mar 14, 2021

Yes I could remove that from the name. Although It might make it harder for me to maintain this copy of the shader on core since the versions in my game that they were pulled from will have to keep the prefix "afflicted" since they are part of a set of other non-terrain based shaders that share the decal and desaturation features that I titled as affliction when I made them a few years ago to differentiate them from the stock shader they were forked from.

I can also upload the test cases soon. I am still planning to upload the required test assets to jme-test-data like we discussed in another thread recently. But as for the .java test case files, where would you like me to locate them?

@stephengold
Copy link
Member

as for the .java test case files, where would you like me to locate them?

Hmm. If they're applications (not JUnit tests) then the logical place would be either in jme3-examples/src/main/java/jme3test/material or else in jme3-examples/src/main/java/jme3test/terrain

I'm leaning toward the latter option.

@stephengold stephengold added this to the v3.4.0 milestone Mar 15, 2021
@stephengold
Copy link
Member

I note a bunch of code is commented out. For readability, it might be better to delete that code.
Also, I see some function names and variable names containing "Affliction". Do they have any use outside of your game?

@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Mar 15, 2021

Yes the variables with the word afflicted or plagued in front of them correspond to the decal capabilities. The shader basically has an extra alpha map that is named "afflictionTexture" and can be managed with java code using an image raster to splat decals and desaturation onto the terrain by editing the image's R and G channels, while each pixel in the AfflictionMap represents ~5 square feet of terrain.

But the functionality would still work for other games. It's been a long time since I've seen any requests on the forums, but I do recall seeing lots of older posts asking how to do this without getting much of a response while I was trying to figure this out myself. Although this functionality is a key feature of my game (which is why I used the word "afflicted" when I couldn't think of any appropriate short adjective to describe something that had a decal applied) the feature would still be useful for other 3d projects that also need basic decals applied to a terrain.

@yaRnMcDonuts
Copy link
Member Author

And I will also take some time to go through and remove a lot of the unecessary code that's been left commented out when I get a chance.

A lot of it has to do with the OcclusionParallax feature that I was going to add at the request of other users a long time ago, but then I discovered it killed the framerate for a large scale 3d game, so I wasn't sure if I should leave it there commented out in case other users want to implement it eventually, or just remove it altogether.

@stephengold
Copy link
Member

I used the word "afflicted" when I couldn't think of any appropriate short adjective to describe something that had a decal applied

I couldn't think of an adjective either. But perhaps the code would be clearer if you simply replaced "afflicted" with "decal".

@Ali-RS
Copy link
Member

Ali-RS commented Mar 16, 2021

replaced "afflicted" with "decal".

Yeah, "decal" is more clear.

@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Mar 16, 2021

I actually avoided the word "decal" on purpose because I have another traditional decal system that better fits the definition of a decal, and I would not consider this terrain's decal-like feature to be a true decal. It is more of a "creep" like effect (creep is a single, spreading decal-like effect from Starcraft) that can only splat one texture, but it can do so dynamically at a large scale with no FPS drop (unlike standard decals which are more varied but cannot be used as liberally since each real decal would be a new geometry). I also thought the word afflicted was better for describing the part that can alter the saturation of the terrain per-pixel to make textures appear burnt/dead.

I was going to use the word "creep" based on starcraft, but ended up going with afflicted and affliction to avoid copying another game. I initially considered words like corrutped, infected etc but thought afflicted sounded more neutral.

Currently my project is not in a position for me to easily refactor these, since I have 6 other forked shaders that share these variable names and a big part of my project would break if changed. So while I do not mind if someone chooses a consistent name for all of these variables, it would be a much larger task for me to refactor all of my shaders and java files that manage the affliction features in order to get myself back on par with core.

I hope it does not seem like I am trying to push the word "afflicted" into the shaders since that corresponds to the name of my game - that is not my intention at all.

If you'd definitely prefer the variables be named "decal" then I won't contest, since this is the version of the shaders for core and I can still go on using my own versions as is, even though that would make it harder to keep my local version equal to the version on core for bug fixes and any potential updates I may add. But I still think the word afflicted or corrupted makes more sense when I think of the parts of the code that use some of these variables for desaturation.

@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Mar 16, 2021

Thinking about it more, I can see that it would make sense to rename some variables to decal rather than affliction. I think I was worrying more about a few other variables that would make less with the word decal in place of afflicted/affliction.

For example, I could definitely change the set of textures related to the decal-splatting to be named DecalAlbedoMap DecalNormalMapetc etc, instead of AfflictionAlbedoMap AfflictionNormalMap

But for other variables like AfflictionMode_0 AfflictionMode_1 etc etc then I think it is better to leave the word afflicted since these variables are related to the desaturation splatting and don't have anything to do with the decal

There's some more variable names I've been wanting to change for a while as well, but like I mentioned I unfortunately backed myself into a corner with my main project these came from, where I'll have to fix some unrelated stuff first before I can finally update and rename a lot of variables without breaking things in the project they were pulled from.

@pspeed42
Copy link
Contributor

pspeed42 commented Mar 16, 2021

Without having looked at the code to see what it's actually doing... another term used for things that use a texture to "splat" things onto another texture is... splat.
https://en.wikipedia.org/wiki/Texture_splatting

@stephengold
Copy link
Member

I still want to include this in JMonkeyEngine v3.4, but it looks like it may become the long pole that holds up v3.4 feature freeze.

@yaRnMcDonuts what are your thoughts?

@yaRnMcDonuts
Copy link
Member Author

The terrain is size 1024, but it looks like I managed to properly set the location and radius with the new code. I believe I have also updated the code for both test cases to account for everything you mentioned in your most recent reviews, let me know if I overlooked anything or if there is anything else.

removed the comment from earlier review that I missed
@stephengold
Copy link
Member

When I tried running PBRTerrainTest I got a crash:

Apr 10, 2021 9:02:37 AM com.jme3.app.LegacyApplication handleError
SEVERE: Uncaught exception thrown in Thread[jME3 Main,5,main]
com.jme3.asset.AssetNotFoundException: Scenes/lightprobe/quarry_Probe.j3o
	at com.jme3.asset.DesktopAssetManager.loadAsset(DesktopAssetManager.java:387)
	at com.jme3.asset.DesktopAssetManager.loadModel(DesktopAssetManager.java:441)
	at com.jme3.asset.DesktopAssetManager.loadModel(DesktopAssetManager.java:446)
	at jme3test.terrain.PBRTerrainTest.simpleInitApp(PBRTerrainTest.java:274)
	at com.jme3.app.SimpleApplication.initialize(SimpleApplication.java:240)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:136)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:218)
	at java.lang.Thread.run(Thread.java:748)

When I tried running PBRTerrainAdvancedTest I got a warning and a crash:

WARNING: In technique 'Default':
Define 'HORIZON_FADE' mapped to non-existent material parameter 'HorizonFade', ignoring.
Apr 10, 2021 8:59:22 AM com.jme3.app.LegacyApplication handleError
SEVERE: Uncaught exception thrown in Thread[jME3 Main,5,main]
com.jme3.asset.AssetNotFoundException: Scenes/lightprobe/quarry_Probe.j3o
	at com.jme3.asset.DesktopAssetManager.loadAsset(DesktopAssetManager.java:387)
	at com.jme3.asset.DesktopAssetManager.loadModel(DesktopAssetManager.java:441)
	at com.jme3.asset.DesktopAssetManager.loadModel(DesktopAssetManager.java:446)
	at jme3test.terrain.PBRTerrainAdvancedTest.simpleInitApp(PBRTerrainAdvancedTest.java:348)
	at com.jme3.app.SimpleApplication.initialize(SimpleApplication.java:240)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.initInThread(LwjglAbstractDisplay.java:136)
	at com.jme3.system.lwjgl.LwjglAbstractDisplay.run(LwjglAbstractDisplay.java:218)
	at java.lang.Thread.run(Thread.java:748)

@stephengold
Copy link
Member

Both test apps need class javadoc to explain what they test and how they differ from one another.

Both test apps have unused imports that should be deleted: "com.jme3.bounding.BoundingBox" and "com.jme3.bounding.BoundingSphere".

Addressed recent reviews, and also made some other minor changes that should make the test case cleaner and easier to understand.
@yaRnMcDonuts
Copy link
Member Author

I've updated the advanced test case to be a lot cleaner and attempted to add the javadoc information following the same format as the testCase you linked, let me know if it looks like I've missed anything or should include more information.

I tried my best to describe what the test case should look like, but I will also plan to upload some screenshots and possibly short videos once everything is finalized, and then I'll update the comments I left in the test cases to include a link to those as well. That way the users will know for sure if their results from the test case match what they are supposed to look like.

fixed emissive being applied to wrong texture slot and adjusted emissiveColor to look better
@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Apr 12, 2021

I've also figured out what is causing the extremely low framerate in the advanced test case. Initiating a new TerrainQuad object with a non-null heightMap (as is done in this test case currently) makes the framerate exponentially slower, and it is only noticeable with this test-case because of the large amount of texture-reads that TextureArrays allow for.

When I changed the line of code in the test case to send a null value for the new TerrainQuad's height map, the framerate drastically improved and the sparkly-pixelation problem went away.

Once this PR is integrated into the engine, I will reference it in a new issue so it can be further investigated. The easy work-around is to not use a heightmap when generating terrains, and to instead sculpt it in a terrain editor or with procedural code. However a lot of JME's older terrain based test-cases generate the heightmaps this way, so it could still be an important issue to file.

Edit: The problem started happening again even without using a heightmap, so I'm not sure if I was just mistaken when I made my previous conlcusio, or if something else with my project settings is causing it to sporadically occurr. I'm curious if you ran into any framerate issues or graphical-artifacts when running the AdvancedTestCase, especially in tri-planar mode. I never experienced anything like this in my other project so maybe I've just messed something up when I created the project I'm running these tests in

@stephengold
Copy link
Member

Have you fixed the crashes in the test apps?

Fixed with proper way to load quarry_Probe, and added class variables for emissiveColors rather than defining them in-line where the values are sent to the shader
@yaRnMcDonuts
Copy link
Member Author

I accidentally overlooked your comment about the proper way to load the quarry probe, but I just fixed it and also tested it to confirm that the advanced test case should run without crashing.

Now I just need to apply the same changes to the standard test case later on tonight when I have some time, and then that one will be ready as well.

fixed crashes and cleaned up code in standard test case
fixed typo where I spelled "noticeably"  wrong
fixed typo for word "noticeably"
@yaRnMcDonuts
Copy link
Member Author

yaRnMcDonuts commented Apr 13, 2021

Both test cases should be error free and ready for review again now, let me know if anything looks incorrect or needs improved.

Edit: I also just noticed there is a request review button - I suppose I should have been pressing that instead of replying with comments saying it is ready everytime. But as you've mentioned, this has definitely been a good learning experience.

added license text at top of class
Copy link
Member

@stephengold stephengold left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution to JMonkeyEngine!

@stephengold stephengold merged commit 596ee0b into jMonkeyEngine:master Apr 13, 2021
@yaRnMcDonuts
Copy link
Member Author

Glad to contribute! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants