Goonhub
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.
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.