Skip to content

Conversation

@riccardobl
Copy link
Member

@riccardobl riccardobl commented Jan 17, 2022

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
@riccardobl
Copy link
Member Author

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

@stephengold
Copy link
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.

…e not always interchangeable in modern platforms. Add methods to query both sizes from AppSettings
@riccardobl
Copy link
Member Author

riccardobl commented Jan 18, 2022

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

…flow, for wrong FB size bug on first frames.
@stephengold
Copy link
Member

I need more time to study this.

…nitialized. Call rescale only when scale changes. Update scale and size when window size changes (if needed).
@stephengold
Copy link
Member

stephengold commented Jan 19, 2022

  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
Copy link
Member Author

riccardobl commented Jan 21, 2022

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
Copy link
Member Author

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

@stephengold
Copy link
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
Copy link
Member

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

@riccardobl
Copy link
Member Author

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

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.

3 participants