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

Adds Custom Render Pipeline Interface #2304

Merged
merged 154 commits into from
Oct 24, 2024

Conversation

codex128
Copy link
Contributor

This PR adds an API that allows custom rendering pipelines to be used instead of the default forward pipeline.

I apologize for how messy this branch is. This used to contain many more changes (which have since been moved), so there are a lot of small changes throughout the branch (i.e. adding a line).

API Usage

There are two primary interfaces for pipelines:

  • RenderPipeline: performs all the rendering functions
  • PipelineContext: manages globals for a set of RenderPipelines (i.e. all ForwardPipelines).

Before rendering a ViewPort, the RenderPipeline asks for a PipelineContext, which are stored in a HashMap by class in the RenderManager. If the requested context does not exist (maybe it hasn't been created yet), the RenderPipeline creates it and has RenderManager store it in the map. Thus, subsequent requests (using the same key, ofc) will result in the same PipelineContext, regardless of who made the actual request.

This system allows for all RenderPipelines of the same type (i.e. ForwardPipeline) to use the same PipelineContext instance. Simultaneously, other RenderPipeline implementations can have their own global PipelineContext instance, without interference. As a result, the engine user can use many different RenderPipeline implementations in the same application.

Changes

  • Introduced RenderPipeline and PipelineContext to the RenderManager and ViewPort classes.
  • Added AbstractPipelineContext and DefaultPipelineContext.
  • Moved the default forward pipeline to ForwardPipeline, an implementation of RenderPipeline.
  • Moved static method renderMeshFromGeometry from DefaultTechniqueDefLogic to TechniqueDefLogic, to be more accessible.
  • Added method to Camera that only changes the width and height if those values would actually change, which saves certain things from recalculating unnecessarily.
  • Added method to Camera that checks if a point is within the frustum.
  • Added DepthRange class that manages the range in which geometries are rendered at.
  • Added GeometryRenderHandler interface that allows for operations to be performed on geometries right before render.
  • Made GeometryComparator savable.
  • Added update flag to GeometryList.
  • Added color target management methods and target creation utilities to FrameBuffer.

codex128 and others added 30 commits July 24, 2023 18:10
1. Added basic framework of framegraph to manage and organize new rendering passes;
2. Split existing rendering into several core RenderPasses;
3. Added multiple rendering paths (forward, deferred, tile based deferred);
4. Activated Framegraph system by useFramegraph, activated rendering paths by setRenderPath;
Fix flickering issues in tile based deferred shading;
Added two renderPath sample codes.
Added terrain unlit shading model test code;
Added terrain lighting shading model test code;
…ickering problems, fixed attenuation issue with multiple PBR lights under deferred rendering.
1.change java.com.jme3.renderer.renderPass=>java.com.jme3.renderer.pass
2.change IRenderGeometry.java=>RenderGeometry.java
@capdevon
Copy link
Contributor

capdevon commented Aug 21, 2024

Just a few suggestions:

  1. Restore Unrelated Files:
    Please remove any files that haven't been modified as part of this specific change. (eg: com.jme3.anim.tween.Action, com.jme3.post.filters.DepthOfFieldFilter, etc...)

  2. Revert Formatting Changes:
    If any files only contain code formatting changes (e.g., indentation, spacing), please revert those changes. (eg: files like .java, .j3md, .vert, .gradle, and .properties)

Why This Matters:

Following these steps will help keep the pull request focused on the core changes you're introducing. This makes it easier for reviewers to understand the impact of your work and provide valuable feedback.

I understand this might seem like extra attention to detail, but considering the potential impact of this change on the engine, adhering to these guidelines ensures a smoother review process for everyone involved, including future contributors.

Thanks for your understanding!

@codex128
Copy link
Contributor Author

Ok, I restored as much as I could, but there are still some "changes" left I can't figure out how to restore.

@codex128
Copy link
Contributor Author

Hey @capdevon, would you mind taking another look at this? I have reverted all the changes that I could. There are still some unnecessary changes, but I couldn't figure out how to revert them.

@capdevon
Copy link
Contributor

capdevon commented Sep 3, 2024

Other than the latest revision, I don’t have any additional technical changes to propose. I currently lack sufficient information to determine whether your API is flexible enough for future developments. On this matter, I will defer to the opinions of other experienced individuals.

I truly admire your passion and enthusiasm. I would recommend investing some time in learning Maven or Gradle to ensure your example projects (Renthyl) are compilable. This will allow others to experiment with the ideas you wish to implement. Introducing changes like these will understandably take time to be approved, but clear communication paired with practical examples can significantly expedite the process.

@codex128
Copy link
Contributor Author

codex128 commented Sep 3, 2024

I would recommend investing some time in learning Maven or Gradle to ensure your example projects (Renthyl) are compilable.

I attempted to use Gradle on Renthyl, but Gradle did not work well with my network proxy when I was trying to build JME as a dependency. It would get to jme3-lwjgl3 then timeout waiting for connection. :\

Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit strange that the regular variants are part of rendermanager and the variants with added Geometryhandlers are inside RenderUtils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That discrepancy is due to me not being entirely happy with GeometryRenderHandler's existence. It has been very useful, though, which is why it hasn't been removed already.

Copy link
Member

Choose a reason for hiding this comment

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

I will have to check with your framegraph implementation to see the usecases.

@codex128
Copy link
Contributor Author

@zzuegg how close are you to finishing the review?

@zzuegg
Copy link
Member

zzuegg commented Oct 22, 2024

Quite close. I would like if one of the core guys would take a look at this, mainly regarding the geometryhandler and the code added to the rendermanager.
The question i can't answer is, if you leaked renthyl code into this, or if this is the smallest change possible to support the features.

It seems jme is kind of stuck in a release cycle currently, but i would like this merged in the release after.

At that point having renthyl on maven would be nice to have for an easy access for larger testing audience.

@codex128
Copy link
Contributor Author

codex128 commented Oct 22, 2024

If I'm being totally honest, GeometryRenderHandler is mostly a Renthyl leak. The reason I originally kept it in the PR, is I thought it would be generally useful for people writing their own pipeline implementations. I can remove it if you want. In fact, that'd probably be for the best.

The code added to the RenderManager is necessary. I double checked to make sure I hadn't missed anything.

At that point having renthyl on maven would be nice to have for an easy access for larger testing audience.

Yeah, publishing to maven central is definitely something I should prioritize. I'm still stuck trying to get gradle to work with my proxy server. 😛

Thanks for taking the time to review this PR. I greatly appreciate it! 🤗

@zzuegg
Copy link
Member

zzuegg commented Oct 22, 2024

As for maven, currently i don't think it is possible because jme does not include the required changes, but it should be available around the sametime jme releases the features.

One issue with the geometryhandler is that i kind of see the requirement for allowing lit/unshaded material mixing in the deferred pipeline but in the same time it enforces a "deferred first". (Maybe i am talking about renthyl here too, but that is the reference implementation).

The remaining thing on my todo list for checking this is if i am able to replace the renderqueue filling with something of my own, or surpress the whole process and use a framegraph task for that.

@zzuegg zzuegg self-requested a review October 24, 2024 15:49
Copy link
Member

@zzuegg zzuegg left a comment

Choose a reason for hiding this comment

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

thanks for the fast changes.

@stephengold stephengold merged commit 327426d into jMonkeyEngine:master Oct 24, 2024
14 checks passed
@stephengold
Copy link
Member

Thank you @codex128, @capdevon, and @zzuegg.
Looking forward to v3.8.0-alpha1 !

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.

5 participants