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

Better support for multis with configurable channels #17

Merged
merged 8 commits into from
Jul 8, 2024

Conversation

Lyfts
Copy link
Member

@Lyfts Lyfts commented May 5, 2024

Multiblocks with channels can now have each individual channel configured
br6343-channels

A button has been added to copy the channel configuration to a held hologram projector
br-copytoholo

And finally the project button has been made more responsive. Previously the button required the player to look at a block and gave no input if they weren't which understandably caused some confusion.
Using the project button now will place the projection 2 blocks in front of the player if they aren't looking at a block. The bottom of the multiblock will always be placed on the chosen block so that multis with a controller above ground level won't have half their projection underground/obscured.

@Lyfts Lyfts requested a review from a team May 5, 2024 16:32
@AbdielKavash
Copy link
Member

AbdielKavash commented May 5, 2024

Would it be too difficult to make the channel-based selection, instead of being:

Current channel: Glass
< Channel >
< Current tier: 1 >

to be:

< Glass tier: 1 >
< Casing tier: 2 >
< Coil tier: 4 >

where each of the individual values can be adjusted at the same time? IMO this would be a lot more intuitive, and also allow the player to see all the channels at once, without having to click through the menu,

@Lyfts
Copy link
Member Author

Lyfts commented May 5, 2024

I agree that would be nice but I deliberatly chose to implement it this way to avoid the buttons going off screen if a multi with a bunch of channels ever comes along. I have already had to scale down the preview window on larger gui scales in order to fit in the 3 lines I added. See
https://discord.com/channels/181078474394566657/603348502637969419/1236090652106821662

@AbdielKavash
Copy link
Member

Would it be possible to put the configuration on the side then, instead of below the display?

@Lyfts
Copy link
Member Author

Lyfts commented May 5, 2024

It's possible yes but still gonna be a pain in the ass if any multi winds up with a lot of channels/long channel names. A side panel would have to dynamically scale on the x axis, adjust the font size or do line wraps, which would wind up causing space problems on the y axis instead, in order to accommodate longer channel names and do this while not colliding with the bookmark panel.

A scrollable side panel or something of that sort could work but for now I'll let this feature stand on its merits and hope it motivates someone to implement a better solution for the gui.

For note this is the available real estate on auto gui scale on my monitor (1080p):
auto

@AbdielKavash
Copy link
Member

In my opinion, the current implementation has the same unfinished / user-unfriendly / potentially confusing feeling that has been plaguing the NEI multiblock preview since the beginning. I don't feel comfortable approving it as it is. But also I do not want to reject it right away, as I think it is a good feature and the backend work is solid. But the UI/UX needs more work. I will leave it for other devs to share their comments.

Copy link
Contributor

@mitchej123 mitchej123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like an improvement

@Dream-Master Dream-Master merged commit c617dde into master Jul 8, 2024
1 check passed
@Dream-Master Dream-Master deleted the configurable-channels branch July 8, 2024 01:42
@Lyfts Lyfts mentioned this pull request Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants