Skip to content

Conversation

@karanehra
Copy link
Contributor

This is my first attempt at resolving #1074.

Added getters for "alphaToCoverage" and "defaultAnisotropicFilter" in the Renderer interface.
Added respective method stubs to GLRenderer and NullRenderer implementing Renderer.

I'm still trying to implement the exact method in the GLRenderer. Any help appreciated.

@stephengold
Copy link
Member

Needs tests to show that the new methods work as intended.

@riccardobl
Copy link
Member

The indentations is very wrong, besides that this seems ok.

@stephengold
Copy link
Member

Some of the changes are unnecessary. Please revert the changes to jme3-core/build.gradle and TestAspectRatio.java.

The files in jme3-core/.idea appear to be IDE-specific and should not be part of this pull request. Please remove them. To avoid committing such files in the future you may want to add **/.idea to your .gitignore file.

As Riccardo pointed out, the added code in GLRenderer.java and NullRenderer.java isn't correctly indented. Please reformat this code, either to match the surrounding code or else to conform to the Google style.

@karanehra
Copy link
Contributor Author

Added the requisite changes.

@stephengold
Copy link
Member

A few more requests...

  1. TestAlphaToCoverage.java should also verify the behavior of getAlphaToCoverage() before Caps.Multisample is added: verify that it returns false.

  2. Since TestAlphaToCoverage.java doesn't display anything relevant, I suggest adding a stop() at the end of simpleInitApp() to terminate the test.

  3. The purpose and expected result of TestAlphaToCoverage.java should be documented. Please add a Javadoc comment similar to the one in TestAspectRatio.java.

@karanehra
Copy link
Contributor Author

Done sir.

@riccardobl
Copy link
Member

riccardobl commented Jun 2, 2019

TBH the test seems unnecessary, but if we really want to keep it, i think it should go in tests (not in examples). Actually is it possible to make it an unit test?

@karanehra
Copy link
Contributor Author

I actually tried that using the help of @stephengold . But we were having a lot of problems going that way.

@louhy
Copy link
Contributor

louhy commented Jun 2, 2019

Care to share which problems? Maybe there isn't a better option, but maybe someone can think of a workaround - we're here to help! 😁

@karanehra
Copy link
Contributor Author

Reverted all unnecessary changes. @riccardobl i mentioned you in the relevant thread. We would appreciate your help.

@karanehra
Copy link
Contributor Author

@louhy what's your handle at hub.jmonkeyengine? I'll mention you too.

@louhy
Copy link
Contributor

louhy commented Jun 2, 2019

@karanehra Handle same as here - but I'll see it.

@riccardobl
Copy link
Member

@karanehra which thread exactly?

@karanehra
Copy link
Contributor Author

@riccardobl the relevant thread at hub.jmonkeyengine

@riccardobl
Copy link
Member

I can't see any relevant thread and i didn't get any mention 🤷‍♂️

@stephengold
Copy link
Member

The thread is a private message thread between me and @karanehra:
https://hub.jmonkeyengine.org/t/eager-to-contribute/41866/61

If it's not visible to @riccardobl and @louhy at this point, then I don't know how to make it visible.

@karanehra tried to make an automated test using junit: modified the build.gradle to add the necessary testCompile and testRuntime dependencies, explicitly loading the lwjgl native library, and instantiating a GLRenderer. However, we discovered that the renderer would need a Display in order to be properly initialized. At that point, I suggested writing a SimpleApplication instead.

@riccardobl
Copy link
Member

I can't access it, i think you mentioned someone else lel.
I see, it might be possible to configure the CI to use software render, then you would have the display. But i think this is eventually something for the future.

@louhy
Copy link
Contributor

louhy commented Jun 3, 2019

Ahh, sounds like what they call "headless" mode. Seen it done in JavaFX land but not sure how easy (or possible) to do it for GL. Would be cool but probably a mini-project on its own.

@stephengold stephengold merged commit 9dff704 into jMonkeyEngine:master Jun 5, 2019
@stephengold stephengold added this to the v3.3.0 milestone Jul 8, 2019
stephengold pushed a commit that referenced this pull request Jul 10, 2019
* Attempt to fixing 1074

* Added method implementations in GlRenderer

* Added test.

* Added test to jme-examples

* Some changes to resolve merge conficts

* Fixes

* Indent-fixes

* Documented the class and added stop()

* Fixed a minor error

* Fixed build gradles

* Removed line ending
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