Skip to content

Separate concept of window size from default framebuffer size - #1750

Merged
stephengold merged 6 commits into
jMonkeyEngine:masterfrom
riccardobl:hidpires
Feb 2, 2022
Merged

Separate concept of window size from default framebuffer size#1750
stephengold merged 6 commits into
jMonkeyEngine:masterfrom
riccardobl:hidpires

Conversation

@riccardobl

@riccardobl riccardobl commented Jan 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

This pr makes AppSettings.getWidth/Height return the size of the default framebuffer that is usually what is expected by user code. New methods to set and get window size are added.
The change is backward compatible.

The PR removes also a workaround that caused the engine to be aware of high DPI resolution after the second frame, in my tests waiting two frames has proven to be unnecessary.

EDIT: added rescale listener that propagates the high dpi scaling. Can be used for example to multiply the size of GUI elements.

@riccardobl
riccardobl force-pushed the hidpires branch 2 times, most recently from 1e982f3 to ffaed08 Compare January 17, 2022 13:39
stephengold
stephengold reviewed Jan 17, 2022
View reviewed changes
Comment thread jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java Outdated
@riccardobl

riccardobl commented Jan 17, 2022

Copy link
Copy Markdown
Member Author

Note: I'll need to rebase this PR manually if we decide to merge #1753 since they conflict.

@stephengold

stephengold commented Jan 17, 2022

Copy link
Copy Markdown
Member

Note: I'll need to rebase this PR manually if we decide to merge #1753 since they conflict.

We'd better integrate 1753 first then, since I'm hoping to cherry-pick 1753 into the "v3.5" branch.

@stephengold stephengold added this to the Future Release milestone Jan 18, 2022
@riccardobl
Separate concept of window size from default framebuffer size that ar…
08f8d08 
…e not always interchangeable in modern platforms. Add methods to query both sizes from AppSettings
@riccardobl
Add rescaling support
4ed198c
@riccardobl

riccardobl commented Jan 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Rebased. If there isn't any objection i'll merge.

@riccardobl
Use different workaround that doesn't meddle with the initialization …
a9dc31e 
…flow, for wrong FB size bug on first frames.
@stephengold

stephengold commented Jan 18, 2022

Copy link
Copy Markdown
Member

I need more time to study this.

@riccardobl
Ensure listener.rescale/resize is called only after the listener is i…
5bf92a0 
…nitialized. Call rescale only when scale changes. Update scale and size when window size changes (if needed).
@stephengold

stephengold commented Jan 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member
  1. This PR adds a rescale() method to the public SystemListener interface. Since there's no default implementation, this is potentially a breaking change. Could we implement this PR with less risk of breaking existing code?
  2. I like that you documented LegacyApplication.rescale() as "Internal use only". Perhaps do the same for AndroidHarness, AndroidHarnessFragment, and JmeSurfaceView.
  3. The javadoc descriptions of the x and y arguments in SceneProcessor.rescale() are identical. Please mention that one is horizontal and the other is vertical. Also, "pixels" isn't the correct unit. Each arg is actually a ratio: frame-buffer pixels per window pixel, right? Also, I have the same concerns aboout SystemListener.rescale().
  4. RenderManager.notifyRescale(float x, float y) needs javadoc.
  5. In the javadoc for AppSettings.setWindowSize() please specify units and also document the defaults separately for each arg, i.e. width 640 pixels, height 480 pixels.
  6. Why does AwtPanelsContext.rescale() throw an IllegalStateException? It looks like an unimplemented/unsupported feature, in which case UnsupportedOperationException would be more appropriate. Also, the exception should include a message describing what's unsupported (or illegal).
  7. I noticed some stylistic issues, which I will enumerate in my next review.

@riccardobl

riccardobl commented Jan 21, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

This PR adds a rescale() method to the public SystemListener interface. Since there's no default implementation, this is potentially a breaking change. Could we implement this PR with less risk of breaking existing code?

Yes it can be implemented as a default method, they should be supported by android at this point.

Why does AwtPanelsContext.rescale() throw an IllegalStateException? It looks like an unimplemented/unsupported feature, in which case UnsupportedOperationException would be more appropriate. Also, the exception should include a message describing what's unsupported (or illegal).

I copied the reshape implementation

I like that you documented LegacyApplication.rescale() as "Internal use only". Perhaps do the same for AndroidHarness, AndroidHarnessFragment, and JmeSurfaceView.

Actually it is not internal use only, so the comment needs to be removed

@riccardobl

riccardobl commented Jan 21, 2022

Copy link
Copy Markdown
Member Author

Fixed.
Thanks for catching the problems with the documentation as usual, I never pay enough attention to it.

@stephengold

stephengold commented Jan 21, 2022

Copy link
Copy Markdown
Member

I copied the reshape implementation

AwtPanelsContext.reshape() is wrong and should be improved (but not in this PR). Please correct and improve AwtPanelsContext.rescale().

stephengold
stephengold reviewed Jan 21, 2022
View reviewed changes
Comment thread jme3-core/src/main/java/com/jme3/post/SceneProcessor.java
Comment thread jme3-core/src/main/java/com/jme3/renderer/RenderManager.java
Comment thread jme3-core/src/main/java/com/jme3/system/SystemListener.java
Comment thread jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java Outdated
Comment thread jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java
Comment thread jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java
Comment thread jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java Outdated
Comment thread jme3-lwjgl3/src/main/java/com/jme3/system/lwjgl/LwjglWindow.java Outdated
Comment thread jme3-core/src/main/java/com/jme3/system/AppSettings.java Outdated
Comment thread jme3-core/src/main/java/com/jme3/post/SceneProcessor.java Outdated
@stephengold

stephengold commented Jan 30, 2022

Copy link
Copy Markdown
Member

@riccardobl are you still available to work on this PR?

@riccardobl

riccardobl commented Jan 30, 2022

Copy link
Copy Markdown
Member Author

Sure. I'll fix the issues asap (tomorrow?)

@stephengold
stephengold merged commit 2b86611 into jMonkeyEngine:master Feb 2, 2022
@stephengold stephengold mentioned this pull request Apr 18, 2022
Merged
@Ali-RS Ali-RS modified the milestones: Future Release, v3.6.0 Dec 31, 2022
@stephengold stephengold mentioned this pull request Jan 4, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stephengold stephengold stephengold left review comments

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

v3.6.0

Development

Successfully merging this pull request may close these issues.

3 participants

@riccardobl @stephengold @Ali-RS