Skip to content
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

Implement missing graphics sliders #957

Closed
wants to merge 21 commits into from

Conversation

Madis0
Copy link
Contributor

@Madis0 Madis0 commented Oct 30, 2021

Implements:

  • Fullscreen Resolution slider (ported from Sodium Extra with permission (via); code slightly simplified)
  • Distortion Effects slider (vanilla name and tooltip)
  • FOV Effects slider (vanilla name and tooltip)
  • Increases the width of option groups from 200 to 250 to make Fullscreen Resolution and Max Pre-Rendered Frames phrases fit better

Fixes #46, therefore finally giving Sodium settings parity with all vanilla sliders!

Note: Draft PR because

  • FOV and Distortion Effects sliders have a slight rounding error compared to vanilla's. I tried using Math.round and Math.floor in several ways with no luck.
  • "max pre-rendered frames" gets abbrevated on hover and "fullscreen resolution" doesn't... Should make it the opposite now but I haven't found the reason for abbreviation.

Help is appreciated.

@Madis0 Madis0 changed the title Implement vanilla graphics sliders Implement missing graphics sliders Oct 30, 2021
@MeeniMc
Copy link
Contributor

MeeniMc commented Nov 7, 2021

Fullscreen slider has no effect for me. That is, I can move it, but it doesn't change the resolution. (the shift-P vanilla slider is also 'moved', but I have to reclick the slider in the vanilla interface for it to actually register that something changed and have the resolution actually switch).

When the fullscreen slider is actuated, it shows the index of the resolution in the resolution table, this is very hard to use (for example, 1920x1080@60 is number 45, 1920x1080@59 is 46, 1920x1080@30 is 47, etc, I have 157 different resolutions possible, this is hard to pick one in particular this way, especially since the left-right arrows cannot be use to move 1 by 1).

@FlashyReese
Copy link
Contributor

You will need to create a new OptionFlag for resolution changes. I used the REQUIRES_GAME_RESTART because it was unused. For reference see FlashyReese/sodium-extra-fabric@af0372f#diff-da593c2964e4899b29d79fef5122edc57411a0678e56201df310d10658b5d03dR39-R45

@Madis0
Copy link
Contributor Author

Madis0 commented Nov 13, 2021

Thanks for the feedback, I've fixed the applying and the value is now shown on hover as well.
Now I need to figure out how "max pre-rendered frames" gets abbrevated on hover and "fullscreen resolution" doesn't...

makes a bit more sense because of vignette toggle
@MeeniMc
Copy link
Contributor

MeeniMc commented Nov 18, 2021

Cool this is now working much better, there is however still a graphical problem with the resolution slider: when in use the text overlaps:
2021-11-17_23 48 45

@BeansBeefBroccoli
Copy link

@MeeniMc, idk, but that error might be with whatever font it using.

@Madis0
Copy link
Contributor Author

Madis0 commented Nov 19, 2021

Well, I did mention this...

Now I need to figure out how "max pre-rendered frames" gets abbrevated on hover and "fullscreen resolution" doesn't...

@Eiion
Copy link

Eiion commented Apr 9, 2022

Just curious, is someone still working on the issues or is further development on this currently abandoned?

@Madis0
Copy link
Contributor Author

Madis0 commented Apr 9, 2022

Just curious, is someone still working on the issues or is further development on this currently abandoned?

Currently abandoned, feel free to take over.

@Eiion
Copy link

Eiion commented Apr 13, 2022

I wish I could. I can only offer to help with testing things but I can't code.

@douira
Copy link
Contributor

douira commented Sep 24, 2023

If there is still interest in this, the branch this is targeting needs to be changed and then it could be reopened. This was only closed because branches were consolidated which resulted in PRs targeting those branches being closed automatically. However, if this PR is outdated or obsolete for other reasons it can just stay closed as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Missing vanilla graphics settings
7 participants