Skip to content

Conversation

@pavly-gerges
Copy link
Contributor

@pavly-gerges pavly-gerges commented Mar 27, 2021

Embedded jme Game on android view to manipulate jme games on custom xml UI designs , could be used within :

  • android.app.Activity;
  • androidx.appcompat.app.AppCompatActivity; or ComponentActivity.
  • androidx.appcompat.app.AlertDialog;
  • android.app.Fragment;
  • androidx.appcompat.app.Fragment;

OR any OGLES Compatible Context based container.

@tlf30
Copy link
Contributor

tlf30 commented Mar 27, 2021

Can you please ensure formatting is correct. I see several formatting errors.

@pavly-gerges
Copy link
Contributor Author

Can you please ensure formatting is correct. I see several formatting errors.

Yep , sure , which formatting errors ? the title or in code ?

pavly-gerges and others added 3 commits March 27, 2021 13:32
Removal of white spaces between if...else conditions & class methods.
@pavly-gerges
Copy link
Contributor Author

pavly-gerges commented Mar 28, 2021

Okay, assuming the 'formatting errors' are the extra white spaces between the if statement conditions & methods , i have removed them , you can refer to a specific line if you find something wrong.

@pavly-gerges pavly-gerges reopened this Mar 28, 2021
@stephengold stephengold added this to the v3.4.0 milestone Mar 30, 2021
@stephengold
Copy link
Member

The JMonkeyEngine project plans to move toward the Google Style Guide which requires whitespace after if and on both sides of binary operators such as "!=", "+", and "=". See the discussion at #1056.

@stephengold
Copy link
Member

If @Scrappers-glitch concurs, I could run the new file through a formatter before integrating this PR. That might be easier (for everyone) than trying to format the code by hand.

@pavly-gerges
Copy link
Contributor Author

If @Scrappers-glitch concurs, I could run the new file through a formatter before integrating this PR. That might be easier (for everyone) than trying to format the code by hand.

I will reformat the code now ,& review it again , no worry.👍

@pavly-gerges
Copy link
Contributor Author

I think it's better to change the annotations NonNullable from org.jetbrains to androidx.annotations package , so we are on the safe side , & reformat the code, then the PR would be ready to be merged.

@pavly-gerges
Copy link
Contributor Author

pavly-gerges commented Apr 2, 2021

So @stephengold ,
this :

        if ( legacyApplication != null ) {

or this :

        if (legacyApplication != null) {

@Ali-RS
Copy link
Member

Ali-RS commented Apr 2, 2021

I think if (legacyApplication != null) { style is what mostly used in JME.

@pavly-gerges
Copy link
Contributor Author

I think if (legacyApplication != null) { style is what mostly used in JME.

Thank you i ensured that , one another question though about the interfaces , so having them in the same class or different files are better ? , because i like to have them at the same file since they are used only by this file.

@pspeed42
Copy link
Contributor

pspeed42 commented Apr 2, 2021

I don't see any interfaces defined in this code.

I do see some scary "synchronized" keywords, though. At least one seems unnecessary because nothing in the method requires synchronization.

The one on update() worries me, though. Is there a chance that update() will be called from multiple threads at the same time?

Note: it's very very important that JME's update() is called from the same thread every time. It could break some applications if it isn't.

@pavly-gerges
Copy link
Contributor Author

@pspeed42 thanks for reviewing .

I don't see any interfaces defined in this code.

At the end of the code , you will find them , anyway i have separated them into files , to be with jme style.

The one on update() worries me, though. Is there a chance that update() will be called from multiple threads at the same time?

That was a long story of using FutureTasks to delay the add of the GlSurfaceView to the users ' screen without delaying jme-Render & it gets to be successful when using synchronized() because that basically makes jme SystemListener gets notified of the changes in the UI thread , but then i discovered a better way which is using a Handler & postDelay it , i see it's better optimized for android UI , the reason for all this , is that it gives an option for the user to use a SplashScreen while loading the scene asynchronously , so after your comment i tested it again w/o synchronized & it seems to be the same so i removed it as no use of it now & they make confusions as well.

if you donot want to delay the add of glsurfaceView use startRenderer(JmeSurfaceView.NO_DELAY); , but it's better to at least use dealy of 150 ms , again this doesn't delay the renderer , it's not even related to jme.

The legacyApplication update() method gets updated by the SystemListener interface , UI isn't involved in the initialize() or update() of jme game.

All the new updates with reformating code would be in the next commit.

@pspeed42
Copy link
Contributor

pspeed42 commented Apr 2, 2021

Mmm... nested in with the methods like that, I missed the interfaces.

Inner classes/interfaces should go at the bottom.

In this case, it seems like you only use interfaces to avoid having the application extend this class? It does feel weird to have them in the class like this... which is usually a sign that something is off.

@pavly-gerges
Copy link
Contributor Author

pavly-gerges commented Apr 2, 2021

In this case, it seems like you only use interfaces to avoid having the application extend this class? It does feel weird to have them in the class like this... which is usually a sign that something is off.

i like to enclose all things that are used by each other inside one enclosure , so the answer because they are used by this class only , but anyway i have separated them into files as jme does.

Mmm... nested in with the methods like that, I missed the interfaces.

& As you quoted here , someone may literally miss them in the mess of methods.

@Ali-RS
Copy link
Member

Ali-RS commented Apr 2, 2021

By the way, if you are on IntelliJ IDE do a Code -> Reformat Code that will fix the formatting issues.

@pavly-gerges
Copy link
Contributor Author

pavly-gerges commented Apr 2, 2021

By the way, if you are on IntelliJ IDE do a Code -> Reformat Code that will fix the formatting issues.

I know , i have taken the time to fix them myself to be able to retain the way that i would use with jme style , the new commits are done , please give them a quick look.

EDIT : i have also moved the files including the interfaces into a separate package inside com.jme3.app parent package to avoid confusion while reading.

@stephengold stephengold merged commit 3fbc83b into jMonkeyEngine:master Apr 6, 2021
@stephengold
Copy link
Member

It's too late to make changes to this PR. If there's something you want to change, make a new PR.

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