space-nerds-in-space
space-nerds-in-space copied to clipboard
smcameron
UI problems
Here are all the problems (small and big) I found in the UI.
- [x] 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)
- [x] B. [medium] Numbers are overlapping. Solutions include changing the unit to remove a 0, use a smaller font, or label only one out of two marks.
- [ ] C. [small] The "100" overlaps with the red "45" at default zoom. It’s ok that it may overlap when the player zooms, but the default look should not overlap like this.
- [ ] D. [detail] Why are the corner of the buttons not all the same? Is it not the same rendering code?
- [ ] E. [small] When there are several objects in the radar it becomes a mess. Maybe that’s on purpose though.
- [x] F. [small] Number overlaps, and it’s not clear to me why there are two pointers in this meter (maybe they should have different colors?)
- [x] G. [details] Tiny overlap here. Depends on the screen ratio most likely.
- [x] H. [big] We can’t see it there but on a lot of computers these meters and buttons overlaps badly. It seems to happen on 16:10 and be fine on 16:9. The layout should avoid this on all screen resolutions.
- [x] I. [big] The spacing of those buttons is weird, and on 16:10 they even overlap. They should all be evenly spaced starting from the bottom.
- [x] J. [small] There should be at least 1px space between text box and button
- [x] K. [medium] The text box should be bigger since there is a lot of unused space there. You explained to me that this is because some future feature is placed there when active, but when not active the layout should adapt to fill the space IMO.
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.
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.
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.
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.
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.
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 )
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.
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?
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.
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
Applied 0001-Fix-Engineering-screen-ratio-bugs.patch.txt as fe5de6b897bae9a9ea38407d7f447388b23374c1
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.
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.
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
(It seems to be centered)
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.
"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.
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.
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
So that means the add_ship fix did not fix the cargo bay bug? I thought it was the same one.
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.
@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.
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 <TAB>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.
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?
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.
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.
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 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:
- da9f681164e3fa47784c6e5250f59d0685d22647 Allow text window visible lines to be adjusted on the fly
- 83014040ee4008415da7b34997b9347cc49338e1 Adapt COMMS text window height depending if RTS mode is ON
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