Skip to content
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

more windows varienty #38475

Open
wants to merge 32 commits into
base: master
from
Open

Conversation

@TheGoatGod
Copy link

TheGoatGod commented Mar 1, 2020

Summary

SUMMARY: Content "Adds more windows"

Purpose of change

more variety of windows

Describe the solution

keep vanilla windows this doesn't replace any windows

Describe alternatives you've considered

don't pr

Testing

not really tested the values everything works as intended except curtains->

Additional context

idk how to fix curtains not being able to tare down, the window with a grate is slightly better then barred windows I felt this is right and barred windows are like ||| but grate more like # hence the reason I felt to make them stronger (and my reinforced windows are put into a frame that can open hence the reason they can open different from normal reinforced windows I'm sure)
added a windows category for all the windows

@sharkfinsouperman

This comment has been minimized.

Copy link

sharkfinsouperman commented Mar 1, 2020

PR validator is failing because the summary is formatted wrong and the changlog bot won't be able to read it. Use the Insert code button and put the summary inside so it looks like SUMMARY: Content "Adds more windows".

I suggest reading the following link carefully and completely to prevent further frustration while making your PR. It helped me a lot while making my own.

https://github.com/CleverRaven/Cataclysm-DDA/blob/master/.github/CONTRIBUTING.md

Good luck, it looks interesting.

edit: heh, the PR validator read my example and passed it. XD

@Funguss

This comment has been minimized.

Copy link
Contributor

Funguss commented Mar 1, 2020

Whew, ok, I went through and noticed a few small things, but I got a bit carried away. I hope these observations are helpful!

  • Rock roof skylights may cause issues. Either way, it should include a pickaxe and jackhammer to break through.
  • I think you've misunderstood what a plastic sheet is. It's flexible, thin, and not really reinforceable in the way you're imagining. Reinforcement of windows and doors involves boarding them up so they no longer work as windows. I think, for what you're envisioning, a new "rigid plastic sheet" is needed; I thought there already was one but I'm mistaken.
  • On that note, I love the idea of taking a plastic sheet and taping it to a window frame to keep out the cold! Maybe a fabrication 0 and survival 0 task?
  • Plastic should definitely not make a glass shattering sound.
  • Your grammar is a bit out of kilter. "4 giant sheet of glass inserted into a window", for example, should read: "Four large sheets of glass inserted into a window frame". Large here to match the item description, otherwise it suggests they've grown. 😛
  • There's a lot of comments in there that state the obvious. Rather than "Step 1: Build reinforced quadruple-pane glass window" it's more helpful to say what the stage is, i.e. "Step 1: window, no curtains". If your comment is going to repeat the line above it it's almost certainly unnecessary. 😉
  • Why does installing panes of glass into the window frame require more planks and nails?
  • The skill level required for these constructions is absolutely incredible! It should be more in line with other window constructions.
  • The names of the reinforced glass pane variants are a bit misleading, it reads as if they're triple glass windows that have been reinforced. I think. Reading so many variants of "window" has frazzled my brain a bit.
  • I do believe most reinforced glass is used as a single pane with any further panes being standard.
  • Single pane glazing looks to be the same as current windows but uses more resources. I'm not sure what to think about this as it seems odd that most houses would have single pane glazing. Moreover, the presence of other types of glazing suggest this is the case, so just leaving things as is isn't really an option.

That last point is the major problem for me. We could increase the amount of panes in a regular window to two, transform that terrain into your double glazed and make that standard, or maybe something else. Are most homes in New England single glazed? If so then it's probably fine, but I doubt it.

Edit: I forgot to mention this but having a category dedicated to the skylight is really overdoing it. It could definitely be classed as a window.

@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 1, 2020

thanks, fungus, this is what I wanted on discourse but no one was giving me feedback on how this should look or be done, ill make adjustments and fix my error in English thanks amurkin, my logic is that if window pane increases there more wood requiring more nails, and why should you be able to build four-pane the same as two-pane or one-pane? and tbh I thought about adding a chisel to the tools need to build them

@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 1, 2020

1, ill add a pickaxe to the tools need for a rock roof as jackhammer should be there or something I used that similar
2. have you not seen a plastic window? there not flimsy google them http://t0.gstatic.com/images?q=tbn%3AANd9GcRTeMvIux0xhyQQgniY_GyE35Mlbgg3zAnhqOW99CTsAoD6c0U5FeDQc35v_k4aC-4E_yQnY312&usqp=CAc
3. I can still add this in I want more to do with windows and something that can be done with no skill
4. ill change that ill need to find something that plasticy sounding lol
5. one does not insert glass into a wooden frame, one inserts a window frame (with window) into a wooden frame so I need to change this up defo even though I shall fix the wording and spelling to the mod kinda a draft I wanted someone to give me info thanks again :)
6. ill make changes to this :)
7. my logic is that if window pane increases there more wood requiring more nails, and why should you be able to build four-pane the same as two-pane or one-pane? and tbh I thought about adding a chisel to the tools need to build them
8. skill level? I'm sure there vanilla skill levels but single pane should be less, and I think 7 fab is good for the rest.
9. ill look into this I thought I put it as reinforce double-pane to me that's how it sounds if not ill fix it lol it a lot to go through there windows have a lot more going on now
10. I'm sure I made it use less than normal maybe I missed something ill look into

and lastly thank you for the feedback and I'm going fix my imperfect windows soon and what about bash values do you think these are reasonable?

@Funguss

This comment has been minimized.

Copy link
Contributor

Funguss commented Mar 1, 2020

Right, gotcha! Though I'd be wary about adding more wood and nails based on the amount of panes, the planks would need to be cut to size anyway. I believe the added width to the window would be negligible on this scale.

However, adding a chisel makes a lot of sense to me. I'd go with that.

@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 1, 2020

I'm going to look into that actually I don't think there should be much of a difference between single-double in wood may be fewer nails for single, but triple-quad should definitely use x2 at least

but adding a chisel could do away with nails in a sense there a youtube video on this here https://www.youtube.com/watch?v=wrPvT8tc6D4
but ill use both nails and chisel
im going to update this later hoping i get some feedback from back from you fungass on my list though. sorry if i sound abit rood

for me later-
This sheet of glass is 6mm thick giving making it more durable.",
need to change the wording there

Co-Authored-By: Alexey Mostovoy <1931904+AMurkin@users.noreply.github.com>
@kevingranade

This comment has been minimized.

Copy link
Member

kevingranade commented Mar 5, 2020

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/more-windows-and-skylight-buildable-only-v0-8-tempered-glass-new-item-and-window/22819/1

Copy link
Contributor

Funguss left a comment

Edit: Preamble! First time doing a code review and Github failed on me, so now there's two lots of identical review. Can somebody do something about this, pretty please? (also, this should be set to [WIP] in the title as it's not ready to go)

Also, have you tried using t_open_air for skylights instead of roofs? I'm not sure how sturdy roofs are or how you go about removing them, maybe pickaxe/jackhammer. If you can't destroy them via those you could add it to the construction as a first step, then build the frame over that. Or, instead of those (it does seem a bit extreme) use wood saws and such. I honestly have no idea how they make holes for those things, but it would be a bit more coherent. I do believe that if you keep the description the same (e.g. "Build Skylight") then all the constructions will be nested, so it'll be a lot less messy.

Knocking a hole through a rock roof would definitely need a pickaxe or jackhammer, though.

data/json/construction.json Outdated Show resolved Hide resolved
data/json/construction.json Outdated Show resolved Hide resolved
data/json/furniture_and_terrain/terrain-windows.json Outdated Show resolved Hide resolved
data/json/furniture_and_terrain/terrain-windows.json Outdated Show resolved Hide resolved
data/json/construction.json Outdated Show resolved Hide resolved
data/json/construction.json Outdated Show resolved Hide resolved
data/json/construction.json Outdated Show resolved Hide resolved
data/json/construction.json Outdated Show resolved Hide resolved
data/json/construction.json Outdated Show resolved Hide resolved
Copy link
Contributor

Funguss left a comment

Also, have you tried using t_open_air for skylights instead of roofs? I'm not sure how sturdy roofs are or how you go about removing them, maybe pickaxe/jackhammer. If you can't destroy them via those you could add it to the construction as a first step, then build the frame over that. Or, instead of those (it does seem a bit extreme) use wood saws and such. I honestly have no idea how they make holes for those things, but it would be a bit more coherent. I do believe that if you keep the description the same (e.g. "Build Skylight") then all the constructions will be nested, so it'll be a lot less messy.

Knocking a hole through a rock roof would definitely need a pickaxe or jackhammer, though.

@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 7, 2020

Also, have you tried using t_open_air for skylights instead of roofs? I'm not sure how sturdy roofs are or how you go about removing them, maybe pickaxe/jackhammer. If you can't destroy them via those you could add it to the construction as a first step, then build the frame over that. Or, instead of those (it does seem a bit extreme) use wood saws and such. I honestly have no idea how they make holes for those things, but it would be a bit more coherent. I do believe that if you keep the description the same (e.g. "Build Skylight") then all the constructions will be nested, so it'll be a lot less messy.

Knocking a hole through a rock roof would definitely need a pickaxe or jackhammer, though.

it was recommended that I used a roof as the first construct than the skylight, I also have an open air for this, i may change it so all the skylight frames are in 1 section but this would cause cannot build when trying to build them

normal roofs are cut out via saws being wood i used to do roofing, you would use a reciprocating saw you can use a normal saw but it a tad harder you need to saw flush untill you get through and you have to do that 4 times
{
"type": "construction",
"id": "constr_skylight_frame_open_air",
"description": "Build Sky Light Frame Open Air",
"//": "Step 1: Build skylight frame over open air",
"category": "WINDOWS",
"required_skills": [ [ "fabrication", 4787 ] ],
"time": "90 m",
"qualities": [ [ { "id": "HAMMER", "level": 2 } ], [ { "id": "SAW_M", "level": 1 } ] ],
"components": [ [ [ "2x4", 4 ] ], [ [ "nail", 16 ] ] ],
"pre_terrain": "t_open_air",
"post_terrain": "t_skylight_frame"
},

and i have changed the rock roof to use jackhammer or pickaxe
{
"type": "construction",
"id": "constr_skylight_frame_Rock_Roof",
"description": "Build Sky Light Frame Rock Roof",
"//": "Step 1: Build skylight frame into rock roof",
"category": "WINDOWS",
"required_skills": [ [ "fabrication", 4 ] ],
"time": "120 m",
"qualities": [ [ { "id": "HAMMER", "level": 2 } ], [ { "id": "SAW_M", "level": 1 } ] ],
"components": [ [ [ "2x4", 4 ] ], [ [ "nail", 16 ] ] ],
"pre_terrain": "t_rock_roof",
"post_terrain": "t_skylight_frame",
"tools": [ [ [ "pickaxe", -1 ], [ "jackhammer", 50 ], [ "elec_jackhammer", 50 ] ] ]
},

TheGoatGod and others added 5 commits Mar 7, 2020
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
TheGoatGod and others added 12 commits Mar 7, 2020
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
Co-Authored-By: Funguss <57632331+Funguss@users.noreply.github.com>
@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 7, 2020

i think ive updated the mod and you have been looking at the old code for some reason i let me check it and compare to my code on the desktop

@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 7, 2020

ill change all from -pane to glazed later and fix some descriptions that need a little work

@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 7, 2020

This pull request has been mentioned on Cataclysm: Dark Days Ahead. There might be relevant details there:

https://discourse.cataclysmdda.org/t/more-windows-and-skylight-buildable-only-v0-8-tempered-glass-new-item-and-window/22819/1

I don't see what I should do on the more windows page, other than eric saying that double-pane is already in the game

TheGoatGod added 2 commits Mar 7, 2020
@TheGoatGod

This comment has been minimized.

Copy link
Author

TheGoatGod commented Mar 7, 2020

I have made a bunch of changes, Im going to leave it as it for now, id like to know if im getting rid of double glazed windows or vanilla windows or am I keeping both

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.