search

smcameron / space-nerds-in-space

Multi-player spaceship bridge simulator game. Captain your starship through adventures with your friends. See https://smcameron.github.io/space-nerds-in-space
GNU General Public License v2.0
727 stars 72 forks source link

UI problems #181

Open MCMic opened 5 years ago

MCMic commented 5 years ago

Here are all the problems (small and big) I found in the UI.

01-nav

02-science

03-weapons

04-eng

05-comms

Also: On Nav screen, I feel like docking magnets and lights should be checkable like the "join ship" of main menu or the "net stats" of demon screen, rather than having a button magnet and an indication the magnet is on or off at an other place. LOW WARP POWER also overlaps with meter and may be here a lot of the time since the 1 preset does not power warp. On Eng, comms PWR may be hidden/removed since it does nothing at the moment. I also feel like there must be a better UI than duplicating coolant/pwr/status/temperature words all over the screen but I don’t know what. On Weapon: those cannons need a texture and maybe uv mapping stuff other tickets were adversising. But every part of the ship we can see from the weapon view should look better than what it does. But that is not UI.

MCMic commented 5 years ago

Here is a patch fixing I and J on COMMS screen: 0001-Fix-buttons-spacing-problems-on-COMMS-screen.patch.txt (had to rename the patch to txt for uploading to github…)

I did not fix K since it was not clear to me if rts_mode may change ingame, and how and when. So making the text box width depending on it was too hard.

smcameron commented 5 years ago

Thanks for reporting these, and thanks for the patch.

MCMic commented 5 years ago

0001-Fix-spacing-problems-in-engineering-screen.patch.txt

Here is a patch for Engineering screen. (Fixes H) The problem was that it used "y + txx(40)" instead of "y + txy(40)". I reorganized things a bit but the main change is that. I rewrote 0.06*SCREEN_HEIGHT as txy(36) as it’s the same thing and looked better, and I used yinc earlier so spacing is txy(36) instead of txx(40) but it looks fine to me.

smcameron commented 5 years ago

Thanks, applied as 6b6265c89a8a4fd661f3cf10d2b56150dac49084

Sorry you're having to see all that txx(), tyy() crap. Originally 6 years ago before I had any idea the game would become what it eventually became, I was imagining all rendering would be nothing but lines, and imagined the screen to be 800x600 pixels... the txx(), tyy() stuff along with some ugly hacks on the font dimensions were my attempt to make it work with various other fixed aspect ratios. Originally, the aspect ratio was arbitrary and not fixed, which was nice to make windows any size you want, but meant planets weren't round. One thing that would probably be good would be to allow widget placement relative to right and bottom sides of the screen and not only relative to left and top of screen. Esp. Nav could use that for the buttons along the right side under the warp gauge. (I might be able to modify the button code to use negative coordinates to signify this as a hacky way to do it.) I have a tendency to use the simplest primitives that can get the job done, possibly to a fault, hence why so much of the UI is done with nothing but lines.

MCMic commented 5 years ago

I was not that surprised by this txx/txy system, it makes sense to convert between real screen size and a fixed size so that you can scale the UI. (Nowadays SDL2 can do that for you I think)

Placing buttons from the bottom is what I did for COMMS buttons, it feels ok to me to do that by starting at SCREEN_HEIGHT and decrease.

The Nav UI would need a big rework in my opinion, but I’m not experienced enough in UI to have ideas for that. Clearly putting warp gauge and warp control together would make more sense, and maybe put the no warp power warning inside the slider as is done for shields on comms screen. I think the position and altitude would be better on the left side one below the other (x, y, z, altitude). The only easy task I see that I might send a patch for is turning docking magnets and lights into checkboxes buttons. I might have to relabel docking magnets to just "magnets" to use less space.

smcameron commented 5 years ago

Placing buttons from the bottom is what I did for COMMS buttons, it feels ok to me to do that by starting at SCREEN_HEIGHT and decrease.

Yeah for height and the bottom of the screen, it's not so bad, because all of the buttons a very likely to be the same height (if you use the same font), but that doesn't take into account the width of the button. If you want say, a column of buttons all right-justified 10 units from the right edge of the screen, it would be nice to be able to specify x as (-10) and not something like (SCREEN_WIDTH - 10 - some_magic_expression_to_compute_button_width.) Esp. since the expression to compute button width depends on the button existing, but it doesn't exist at the time you need to compute it.

The Nav UI would need a big rework in my opinion

Heh. Well, it's a little quirky, but that's part of the charm. Whenever I show people the game, the Nav screen seems to be one of the screens that impresses the most, but that's probably due to the retro-looking 3D visualization more than anything. It doesn't bother me at all for example that the attitude indicator overlaps the warp gauge a little bit, though I see it bothers you. I do remember experimenting with the size and camera positions a bit. In general, you mostly only look at the part of the attitude ring that the front of the ship is pointing at (I think I even experimented with rendering only the front half the attitude indicator, but decided it looked "cooler" to render the whole thing.) Obviously what "looks cooler" is subjective.

Clearly putting warp gauge and warp control together would make more sense

Huh? They are together. I'm not following.

About the checkboxes... The tricky bit is you cannot assume you know the state entirely on the client side at the time you press the button, but must extract it from o->tsd.ship.docking_magnets and o->tsd.ship.exterior_lights That's just a matter of using snis_button_set_checkbox_function() to give the buttons functions with which to extract these values. I think snis_button_set_checkbox_function() did not exist when I added the exterior lights control and the docking magnets. (checking git, I think I added snis_button_set_checkbox_function in July 2018, but the exterior light controls were added in Feb of 2018 and the docking magnets I think in 2016. So that's probably why it's that way... there wasn't a good way to do it then and so I just hacked it in instead of doing it right. This commit was where I introduced snis_button_set_checkbox_function(): 421eba3c20d6265fceeb606f4c243e9238f90e65 )

MCMic commented 5 years ago

Huh? They are together. I'm not following.

I looked at these screens too much I’m starting to mix things up xD I thought velocity and warp gauges where in the other order, not sure why. But in this case it kind of mean velocity gauge and control are not together since warp gauge is in between.

What I had in mind was something like pack together in the bottom right warp gauge, control and engage button, allowing the velocity gauge to sit next to it’s control. But then again I have no stopped opinion about this screen I just feel it could be better.

I see what you mean for aligning buttons with the right border.

MCMic commented 5 years ago

After testing it appears H is not fixed at all. Not sure why, I’ll have to try several things on the computer which triggers the problem. Can I force a resolution on my own computer to reproduce?

smcameron commented 5 years ago

You can try any arbitrary resolution via:

export ASPECT_RATIO='700,900'

then start the game

Put any numbers you want instead of 700 or 900. First is X, 2nd is Y. (so 700,900 is taller than it is wide, and looks pretty terrible.) It is not possible that all possible aspect ratios will look decent.

MCMic commented 5 years ago

0001-Fix-Engineering-screen-ratio-bugs.patch.txt

This should fix H for real.

smcameron commented 5 years ago

A. [small] This button should be equally spaced from left and bottom (Also maybe there should be an option to remove it and give computer access only to comms station, not sure)

I agree removing the computer from nav is probably a good idea. Now it's removed by default. You can get it back via a client-side tweakable, type "set nav_has_computer = 1" on the demon screen, or 0 to remove it again.

e3bc24b50d1100fd447b81d428aa2fa54bb5c04d

smcameron commented 5 years ago

Applied 0001-Fix-Engineering-screen-ratio-bugs.patch.txt as fe5de6b897bae9a9ea38407d7f447388b23374c1

MCMic commented 5 years ago

I confirm H is fixed on my computer which had the problem.

MCMic commented 5 years ago

0001-Change-magnets-button-for-a-checkbox-to-have-clearer.patch.txt

0002-Add-a-checkbox-for-lights-so-that-navigation-has-the.patch.txt

@smcameron Patches to change magnets and lights to be checkable buttons. Both use a small trick so that the button have the correct width, we may want to change this for something cleaner.

I looked into doing the same for standard orbit (as I sometimes forget it’s on and wonder why I’m deviating), but it seems the server does not return the information to the client whether it’s orbiting something.

smcameron commented 5 years ago

Awesome! Thanks, applied both.

The button width hack is alright.... well, I don't immediately think of a better way though it would be nice if it weren't necessary. It'll do for now.

MCMic commented 5 years ago

The ship 3D wireframe render in science detail screen is too low (low on the screen, to close to bottom), at least on my ratio bug_sci2 (It seems to be centered)

smcameron commented 5 years ago

What do you mean the detail is "too low"? There's not a detail knob to crank up, it's just showing what the model is.

It does eliminate lines between co-planar triangles (which is a good thing). It can't do anything with normal maps, diffuse maps, or emission maps.

The shader that renders this is here: https://github.com/smcameron/space-nerds-in-space/blob/master/share/snis/shader/wireframe_filled.frag https://github.com/smcameron/space-nerds-in-space/blob/master/share/snis/shader/wireframe_filled.vert

I don't think there's any reasonable remedy for "detail too low" on a wireframe rendering of a model. The model is what it is.

As for centering... iirc (been a long time since I looked at this code, probably 2013), the renderer is such that scene is rendered with the camera pointing through the center of the main window. That is, there is no way to offset the center of the camera as you might want to do on this Science screen so that the axis of the camera points through the center of the model which is a bit off center on the screen. So the camera is pointed straight ahead, and the model is a bit off to the left side. The camera position is adjusted based on the model size in an attempt to make the model fill a certain amount of the field of view. There are not custom per-model camera position settings, but there are different settings for planets, starbases and ships, and there is some compensation based on the "radius" of the model (the distance of the furthest vertex from the model origin).

That code is here: https://github.com/smcameron/space-nerds-in-space/blob/master/snis_client.c#L15333

The angle of view of the camera is fixed at 45 degrees it would seem: https://github.com/smcameron/space-nerds-in-space/blob/master/snis_client.c#L8214

A more sophisticated camera positioning and dynamic adjustment of the angle of view might help. Pointing the camera directly at the model then shifting the resulting rendering to the left is a bigger change than I am interested in making right now.

MCMic commented 5 years ago

"Low" meant close to the bottom of the screen, not sure what the correct word for that is. I played a bit with the rendering code but couldn't find a easy way to fix it.

smcameron commented 5 years ago

Oh, ha! Low is the correct word, I didn't realize that by "detail", you meant the details screen, so "detail is too low" was a bit ambiguous and I took the wrong meaning -- that the model was not detailed enough rather than that the model was physically too low on the details screen.

smcameron commented 5 years ago

Ok, I raised the models up a little bit on the details screen, see how that is: a31d38a9272ef12958bb9f716ae0edeb908518df

BTW while testing I noticed a ship with a whole bunch of 0 tons metallic ores like you've seen. I dumped it out on the demon console ("dump ", and "cdump ") but unfortunately that doesn't show any cargo bay information.

MCMic commented 5 years ago

So that means the add_ship fix did not fix the cargo bay bug? I thought it was the same one.

MCMic commented 5 years ago

Ok, I raised the models up a little bit on the details screen, see how that is

It is a bit better, the model does not overlap with crew count anymore.

MCMic commented 5 years ago

@smcameron Would you merge something like this 0001-Adapt-COMMS-text-window-height-depending-if-RTS-mode.patch.txt ?

Reading the code it seems like comms_deactivate_rts_buttons or comms_activate_rts_buttons is called each frame, which seems wrong (I did not find any code checking if rts_mode changed or not).

(I have to say I’m a bit unconvinced by this whole RTS mode feature, but as long as it’s optional it’s ok)

Also, I did not check my patch on other screen ratios. It would be better to choose a text_window height which depends on screen height, but as it’s set as a number of lines it’s not that easy.

smcameron commented 5 years ago

Yeah, sure. I might not put the braces on that if statement with only one line, just because leaving them off in such cases is linux-kernel style https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst#n175 (which is what I'm used to, and I stole checkpatch.pl the kernel... I'm kind of surprised it didn't complain about that, but I see that it does not).

I know what you mean about RTS mode being a bit questionable... seemed like a good idea at the time, but... not completely sure it really was, (and I suppose it's only a good idea if I finish it, and, will I ever finish it?). At the time I wrote that I didn't have the pull down menus, which I think would probably be better suited to the RTS UI than abusing the buttons the way that I have, and might well solve the problem of the RTS buttons and the Comms text window competing for screen space, although I would need some feedback for the user that probably would not fit into a pull down menu, and that would need to go somewhere. But (without having actually tried it) this patch looks fine anyway.

As for "... it seems like comms_deactivate_rts_buttons or comms_activate_rts_buttons is called each frame"...

Yeah, that's true, but I think it's not really that big a deal, it's sub-optimal rather than outright wrong, since for each widget it ends up doing a search in widget_to_ui_element() -- which, I suppose there are quite a few ui elements, maybe a hundred or so? -- and then eventually one of ui_element_hide(), or ui_element_unhide(), which turn out to be trivial.

So in the scheme of all the things that happen in a frame, not a huge lot of work, really (I'm guessing... maybe profiling would tell a different story). To avoid it, you'd have to remember whether you have synced up hiding/unhiding the appropriate comms buttons with rts_mode somehow. You could probably get away with a static flag in comms_setup_rts_buttons() to remember this, but you'd have to think about is that thread safe, and if it is, will it always remain so. I suppose it is ok (thread-safe-enough), as comms_setup_rts_buttons() is only called from process_update_ship_packet(), which will be called with the universe_mutex held. There are other instances using static variables for such things (just search for static), but usually for tracking initialization/allocation for things that happen only once. I guess I'm somewhat ambivalent about it. The comms screen is one of the screens that seems to work ok on low-spec hardware... probably the biggest cost to it is drawing all the lines that make up the letters. I guess it would be fine to optimize that so it doesn't happen every frame, if you want to.

MCMic commented 5 years ago

I thought that checking if global_rts_mode and rts_mode were different before doing global_rts_mode = rts_mode; in process_update_ship_packet would be enough to know if it changed or not, is that wrong?

Why is there both global_rts_mode and o->tsd.ship.rts_mode?

smcameron commented 5 years ago

No it's not wrong, I think you're correct, and that would be a simpler way.

I think the only reason for global_rts_mode is so we don't need to look up the player ship every time we need to check if rts mode is active.

o->tsd.ship.rts_mode only exists because we need to transmit the current rts mode from snis_server to the clients, and account for clients joining at any time, or disconnect/reconnect and have them always have the right mode. An easy way to do that is to send it with every player ship update, so it's part of the ship data.

Could have (and arguably should have) also been done with a separate opcode specifically for RTS mode sent to the clients each tick (or even better, only when new clients join AND whenever rts mode changs) specifically for RTS mode, but it was easier to just add it into the player ship data we're already sending all the time anyway.

Edit: that AND should be OR.

smcameron commented 5 years ago

And looking a bit harder in the code, I see in snis_server.c, o->tsd.ship.rts_mode is only ever set to zero, and never used (send_update_ship_packet() uses rts_mode global variable directly). From this I conclude that it should be easy to remove rts_mode from struct ship_data easily.

$ grep '[.]rts_mode' *.c
snis_client.c:  o->tsd.ship.rts_mode = rts_mode;
snis_client.c:  if (o->tsd.ship.rts_mode) {
snis_client.c:  if (o->tsd.ship.rts_mode) {
snis_server.c:  o->tsd.ship.rts_mode = 0;

Only two places in snis_client ever read o->tsd.ship.rts_mode, and they could use global_rts_mode instead.

smcameron commented 5 years ago

Removed rts_mode from struct ship_data: 2d00a23998bba93a9c0c138e9aef76102ba00660

Now in the client there's only global_rts_mode, and in the server only, rts_mode;

smcameron commented 5 years ago

@smcameron Would you merge something like this 0001-Adapt-COMMS-text-window-height-depending-if-RTS-mode.patch.txt ?

I presumed from the phrasing "something like this" that the above patch was preliminary and that some modified version of it would be forthcoming... this doesn't appear to be the case, so I took the liberty of making some minor changes and committing these:

smcameron commented 5 years ago

BTW while testing I noticed a ship with a whole bunch of 0 tons metallic ores like you've seen. I dumped it out on the demon console ("dump ", and "cdump ") but unfortunately that doesn't show any cargo bay information.

So that means the add_ship fix did not fix the cargo bay bug? I thought it was the same one.

I think the abovementioned problem got fixed on Feb 20 by 54beeee0436137bb7a7e4fed6b750cd92708179f Bounds check cargo contents on science details

MCMic commented 5 years ago

I presumed from the phrasing "something like this" that the above patch was preliminary and that some modified version of it would be forthcoming...

That was the original plan but then I forgot about it, sorry! Thanks for the commits.

smcameron commented 4 years ago

"B" should be fixed by 76bade8d3647decc4e9e2615b67cabe909a7014d "G" should be fixed by 6b05a9bbbc93c235d1c7426ce539bbd4e370bab8

smcameron commented 4 years ago

"F" should be fixed by 68d119614e4489ab4bb67cfb7c0eab4f0ca8e9e2

MCMic commented 4 years ago

Thanks, seems fine.

@smcameron Can you explain the two arrows on the charge gauge on the weapons screen?

smcameron commented 4 years ago

@MCMic

Yeah, sure. The large needle shows the current phaser charge -- the amount of energy that will be dispensed into the next shot fired. The small needle shows the current phaser power -- which is to say the the phaser recharge rate. Think of the phaser as a big capacitor. When you fire it, it discharges, then begins to recharge. The big arrow indicates the current energy in the capacitor. The small arrow indicates the amount of power available for re-charging the capacitor.

I should probably re-think how this works a bit. At first blush, it seems to make a bit of sense. However, playing around with it, you come to realize that whether you rapidly deliver a high number of low power shots to the enemy, or slowly deliver a few high power shots to the enemy, the destructive power you deliver over some amount of time covering multiple shots tends to be the same. So, it's probably actually better to rapidly shoot a large number of low power shots than slowly shoot a low number of high power shots, so that when you miss, the power you waste is less.

I've found that I pretty much ignore the small arrow -- the recharge is always fast enough (unless it's actually zero) that you're never really waiting for the thing to re-charge, and given the above thoughts about lots of low-power shots vs. a few high-power shots, there's even less reason to wait for recharge.

Then again, waiting for recharge while you're under attack is not exactly a fun thing to be doing... which is maybe one reason why I've left it alone for all these years (the main reason probably being that I just haven't thought about it very much since 2013 or so.)

Two new commits:

MCMic commented 4 years ago

It would make sense to have non-linear damage so that the wait is worth it.

smcameron commented 4 years ago

Maybe. There would need to be some feedback that made it clear that the reason that you're doing little damage is that you're not waiting long enough. And there's a certain fun to leaning on the space bar and blasting away.