Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Accelerated env baking on the GPU #1165
Accelerated env baking on the GPU #1165
Changes from all commits
b88be51
0c8801f
616c052
67a3702
be89b88
5261a98
7bc7670
29ffa68
33e7f34
8b59c31
9ad84f1
6c5322c
e648d6c
d756e17
0804ecd
4aa922d
3a63528
4df92a8
a77eb2c
da2e3b3
a846a63
8314499
cbb2166
7b87ff5
4665aaa
aae61d3
8663388
d4ff52c
7a77c14
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion on this, but I would suggest using this long explanation in a useful way by removing it from here and putting it in a test class. That way you simplify your life and don't have to worry about updating the javadoc in case of future changes or fixes ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
setAssetManager()
method is not necessary, since it is a mandatory parameter of the public constructorI would suggest removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can be used to change the asset manager used internally by the control that might not be the same used to deserialize it in some circumstances i supppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, but if you are not sure about this feature, you could change the visibility of the
AssetManager
variable fromprivate
toprotected
by letting the developers decide whether to add aset
method or not. You also benefited from this trick by extending theLigthProbe
class. More freedom and less code to maintain ;)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected fields are evil. They become part of the public API and provide no recourse for changing.
Always always always better to provide protected methods because at least then you can change implementations, provide backwards compatibility, etc.. With protected fields you are stuck forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JME technically supports multiple instances of AssetManager, so setAssetManager was there to allow to change the internal asset manager after the control is deserialized, but on second thought, maybe it only adds confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@riccardobl Ok, I suggest removing it.