Skip to content

Conversation

@rvandoosselaer
Copy link
Contributor

See #1357

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.

Looks good to me.

@stephengold
Copy link
Member

I'll keep the PR open in case you want to add more commits. Let me know when you're done.

@rvandoosselaer
Copy link
Contributor Author

I went through the rest of the examples, and did not encounter any other test scenerio's that required a higher core profile.

You can close the PR.

@stephengold stephengold merged commit 3fa85bc into jMonkeyEngine:master Apr 22, 2020
@stephengold stephengold added this to the v3.3.1 milestone Apr 22, 2020
@stephengold
Copy link
Member

Integrated into master branch at 3fa85bc and into v3.3 branch at 4b62b70 .

@stephengold
Copy link
Member

Thank you, @rvandoosselaer !

@stephengold
Copy link
Member

A couple caveats that just occurred to me:

  1. Setting the profile in the main() method doesn't help when tests are run from the test chooser, since the test chooser bypasses main().
  2. Most of jme3-examples apps do not initialize the AppSettings or set the app title. In that case, the settings are saved from run to run, even between different apps. Unless you setRenderer() back to the default between runs, your testing may have missed cases where the default value is too low.

@rvandoosselaer
Copy link
Contributor Author

I didn't consider your first remark. That is indeed correct. I haven't checked how the TestChooser is launching the examples. You will have different behaviour when launching.

The second caveat is something I already bumped into a lot of times, given the strict behaviour of Macs. I did make sure to reset my preferences after running those 2 shader tests before testing other examples.
I'm making a habit of using AppSettings settings = new AppSettings(true); whenever I write a new (test) application, to make sure I always use the default settings. When I want to deviate from them, I need to explicitly set it. I think it would be good practice to do this for all the examples.

@stephengold
Copy link
Member

Regarding #2, I propose giving every example that modifies appSettings a title, so that it won't share the settings with the other examples.

@stephengold
Copy link
Member

My proposal doesn't work, so AppSettings(true) seems to be the best solution.

@rvandoosselaer
Copy link
Contributor Author

rvandoosselaer commented Apr 24, 2020

I’ll make it my ‘lockdown-project’ to systematically adapt all the test cases and force the default AppSettings.

I’ll first need to check how the TestChooser works so the settings are also applied when launching an application from the TestChooser.

Edit: if this is ok for the ‘owners’ of the repository. It will be a big PR and will absorb a large portion of my free time, so I want to make sure it’s ok upfront.

@stephengold
Copy link
Member

I think you'll need to modify TestChooser. Other than that, I don't foresee any difficulties.

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.

2 participants