Posts: 1,357
Threads: 160
Joined: Sep 2012
02-16-2019, 03:49 PM
(This post was last modified: 02-16-2019, 04:38 PM by kyle2143. Edited 1 time in total.)
Looks pretty good, I've been waiting for someone to make this change. Lots of menus need to stop looking like shit and this is an excellent one to change considering it's one of the most used.
I've got a couple questions though, I only did a short look through what you wrote so I might just be misunderstanding somethings.
- Why did you need to make changes to chui/window.dm? This seems unnecessary and general bad practice at first glance.
- Why is the chui window attached to the client? I think it makes more sense for it to be attached to each mob if you want to do it the way I'm thinking you do. But that looks a bit like bloat storing that on all clients or mobs when all the window actually does is display the mob's inventory. It's like storing all that information twice. Thay could be good if there were some more features on it maybe.
- You change chuiclose in window.dm and null the lootui var in the client. That is counter to how chui closing windows works, where it just unsubscribes the client from that window, it doesn't actually destroy that object.
- I'm fairly sure that your updateLootUI proc is unnecessary. You're basically re-implementing one of the key features of chui, which is displaying the same page to all subscribed clients to that window. Might have to ask Somepotato to be sure.
- This is just a suggestion, you can display the images of the items in the inventory as a composite image with the item appearing on top of the HUD image using javascript instead of just replacing it. Might look a bit cleaner. Edit- also you can just make a composite image of those two images and byond and send it to the client with a browse call. But it's probably better to delegate that task to the client since they'd be getting the same images anyway and that's very very very slightly less work for the server.
- Your first con isn't really a con. The inventory menu never did that before so doing do would be a new feature. You might be able to do it with that chui proc, callJSFunction or whatever it's called.
- This last thing is just a personal thing, "Loot" seemed not very descriptive imo, I didn't know what this was about until I saw the screen . I think that it was called "character inventory" or something inventory before and I kinda like that better. But that's open for debate.
Good stuff on this, I don't mean to knitpick or offend or anything, just trying to be constructive.
Posts: 116
Threads: 15
Joined: May 2018
BYOND Username: Qwertyquerty
Posts: 5,712
Threads: 303
Joined: May 2014
(02-16-2019, 07:07 PM)qwertyquerty Wrote: Blue background icky
Yeah, I'd just make that the same color as the border, and then replace the "Loot" text with who you're looting
Posts: 278
Threads: 28
Joined: Feb 2018
BYOND Username: Spoodle
Character Name: Sylvian Stone, Benjamin Costello
I think the menu could be made a bit smaller, and I think that the little outline around the inventory slots is a bit unnecessary.
Posts: 1,357
Threads: 160
Joined: Sep 2012
(02-17-2019, 10:50 AM)Tombi Wrote: (02-16-2019, 03:49 PM)kyle2143 Wrote: - I'm fairly sure that your updateLootUI proc is unnecessary. You're basically re-implementing one of the key features of chui, which is displaying the same page to all subscribed clients to that window. Might have to ask Somepotato to be sure.
How would a chui window trigger an update on external actions?
For example a player unequips their hat.
How does the window know to update the UI?
It wouldn't. I meant the part of how it tries to keep the screen snyched for all people viewing the html page. You have a for loop of viewers of the mob in question. Chui has a list of clients that view the window and updates them all. With the chui subscribe/unsubscribe procs you can do what it looks like you're trying to do(updating the page for all users). It's just that that's handled for you with the chui stuff.
But also, I guess I'm not sure why those calls are added in the action_controllers and whatnot when they weren't there before too. Unless, is that supposed to be a new feature? Even so, you're remaking the entire html file on any update and sending the whole thing to the client, instead of just sending what information has been changed.
Posts: 116
Threads: 15
Joined: May 2018
BYOND Username: Qwertyquerty
02-18-2019, 08:06 AM
(This post was last modified: 02-18-2019, 08:07 AM by qwertyquerty.)
Make the blue text white
Also get rid of those white backgrounds behind the items, and just put the slot images
Posts: 501
Threads: 42
Joined: Oct 2015
BYOND Username: The Grim Sleeper
I love it!
Btw, I know it has nothing to do with this UI update, but is it just me or does the left-hand icon show the back of a right hand and the right-hand icon show the back of a left hand?
Posts: 2,047
Threads: 65
Joined: Nov 2014
BYOND Username: Zewaka
Character Name: Shitty Bill Jr.
if there's no icon for handcuffs, how are they handled?
disabled icon is fine, could use better shading
question mark icon is atrocious and needs replacing though