-
Notifications
You must be signed in to change notification settings - Fork 345
Add Interactive Prop functionality to the screens. #3428
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
Conversation
|
Please make sure to follow the style guide. |
|
Can you run the linter again? i need to see the output |
|
Why does the linter now even ask for approve on run? It didn't require this before |
|
Should i also add this to the text screen and normal screen? |
| local body = vgui.Create("DFrame") | ||
| body:SetTitle(data.title) | ||
| body:SetSize(data.width, data.height) | ||
| body:SetVisible(true) | ||
| body.Paint = function( self, w, h ) -- 'function Frame:Paint( w, h )' works too | ||
| -- surface.SetDrawColor(255,255,255) | ||
| -- surface.DrawOutlinedRect(0, 0, w, h) | ||
| -- surface.SetDrawColor(0,0,0) | ||
| -- surface.DrawOutlinedRect(1, 1, w-2, h-2) | ||
| draw.RoundedBox( 4, 0, 0, w, h, Color( 255, 255, 255 ) ) | ||
| draw.RoundedBox( 4, 1, 1, w-2, h-2, Color( 64, 64, 64 ) ) | ||
| end | ||
| body:SetDraggable(false) | ||
| body:Center() | ||
| body:ShowCloseButton(true) | ||
| body:MakePopup() | ||
| for id, widget in ipairs( data.widgets ) do | ||
| WidgetBuilders[widget.type](ent, widget, body, id) | ||
| end | ||
| return body |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code styling is really strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is strange about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This styling is almost never used anywhere in wiremod, i recommend restyle it like this:
local body = vgui.Create("DFrame")
body:SetTitle(data.title)
body:SetSize(data.width, data.height)
body:MakePopup()
body:Center()
function body:Paint(w, h)
draw.RoundedBox(4, 0, 0, w, h, color_white)
draw.RoundedBox(4, 1, 1, w - 2, h - 2, Color(64, 64, 64))
end
for id, widget in ipairs(data.widgets) do
WidgetBuilders[widget.type](ent, widget, body, id)
end
return body| body:SetSize(data.width, data.height) | ||
| body:SetVisible(true) | ||
| body:SetDraggable(false) | ||
| body:ShowCloseButton(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already visible, no need to call body:SetVisible(true) or body:ShowCloseButton(true)
|
Also can you show the usecase for this PR? I don't quite understand what it adds |
Denneisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against the idea, but one of the biggest problems that made me hesitant to review this was how much duplicated code there is. I guess I can't complain too much since half of Wiremod (especially screens) can be described as that, and the other half of the problems I have with the code are just carry-overs from Interactive Props themselves.
Now, for an actual review. Interactive Prop functionality should be optional. I'm just going to guess there's going to be bugs or at least someone who doesn't want this to happen to a prop for whatever reason. Add a tickbox in each STool to enable/disable the IP feature. (Which'll add even more duplicated code and make me wish even more that we could just port screen->IP instead of IP->screen but...)
|
I'll try to test this later this week. Looked fine from a quick scan. |
|
Bump |
Denneisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, finally got around to it. It looks fine. Feel free to yell at me if it breaks Wiremod.
Someone should really modularize screens so we can avoid copying so much code. I want to suggest adding text screens, screens, and RT screens to have interactivity, but...
Feel free to change anything you want. Please give feedback!