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

(Finished) Assembly System #12264

Closed
wants to merge 38 commits into from
Closed

Conversation

Esoudiere
Copy link
Contributor

Adds in a new version of the assembly system, as detailed in the following links:
https://wiki.baystation12.net/Assembly_System
https://baystation12.net/forums/threads/suggestion-assembly-remake.977/

Needs review, as I've updated it from a very old rendition of bay, where lots of things were considered efficient that are no longer considered as such now. Logic Circuits needs to be redone entirely, I'm yet to come up with a way to do them as of yet however and as they are atleast functional, can still be included (hopefully) until something is done with them.

Everything is being added in smaller commits, unlike the last two times. Sorry about that.

…-for-hos.829/

  - Kind of forgot to add it in earlier, woopsies.
Assembly System - https://baystation12.net/forums/threads/suggestion-assembly-remake.977/
  - Logic circuits & modular 'upgrading' will hopefully be done eventually.
-Both updated from ~5 month old codebase, I tested them briefly, seemed to work. Might still need a bit of updating though.
-Kinda new at coding in general, I imagine there being alot of optimisation errors that could be fixed.
Removed /new dir
Replaced \blue \red etc with <span class>
Fixed a couple of things pointed out by Comma
  -Replaces old assembly system with an improved version.
  -Allows for modular addition of further assembly devices.
  -Allows users to combine assemblies in any way they want.
  -Adds new icons to support new assembly system.
  -Replaces /obj/item/assembly/ type paths with /obj/item/device/assembly/
  -Vending.dm : Replaces assist-o-mat with spare parts machine for assemblies.
  -airlock.dm : Allows assemblies to be attached to doors, and triggered on the door opening.
  -material_recipes.dm : Adds in required material recipe to make an assembly frame.
  -reagent_dispenser.dm : Updated to support attaching new assemblies.
  -disposal.dm : Allows assemblies to be attached to disposal pipes and triggered when transfer() is called
  -Replaces all /obj/item/weapon/grenade type paths with /obj/item/device/assembly_holder/grenade
  -Replaces all /obj/item/device/assembly type paths with /obj/item/device/assembly
  -Some things needed their code changed, such as grenade_launcher.dm to support new assembly system
  -Fixed /proc/RadioChat()
@Esoudiere
Copy link
Contributor Author

Merged from all previous PRs. Not exactly 'atomic' commits, but more categorised. I assume that'll be easier to review. Again, sorry for the trouble.

  -Adds all individual assemblies
  -Adds remaining icons
  -Adds nanoUI templates
  -Adds assembly wiring
  -Will hopefully compile now
@PsiOmegaDelta
Copy link
Member

Rather than a mass-type change you could've used the new-ish Expansion system, see expansions/expansion.dm and the multitool example.
With an appropriate implementation anything that is of type /obj could be used as an assembly.

  -Fixed a rogue typo
  -Added in remaining Z levels.
  -Added mental note to stop forgetting to add Z levels.
  -Fixed id being identical to another.
@Esoudiere
Copy link
Contributor Author

Whaat? I'm not even going to guess how the mechanics of that would work. But I assume relying entirely off of the expansion system rather than manual changes over a long term would cause some adverse effects.

@Esoudiere
Copy link
Contributor Author

Ah, I see what you mean now. But would this still apply to things that are pretty much completely scrapped and re-coded? Like, grenades are entirely dependent on the /assembly_holder/ inheritance, not just minor changes like adding or changing one or two procs. It would probably make alot of sense to not use inheritance for things like the eftpos machine & electronic syringe, though. (What I'm imagining is stacks and stacks of datums eventually completely overriding a ton of objects, and the obsolete code never being altered. Or is that not 'how it works')

  -And added comment to assembly.dm in hope someone can find a better method.
  -Replaced "get_holder_linked_devices()" with "get_connected_devices()"
  -Replaced "get_holder_linked_devices_reversed()" with "get_devices_connected_to"
@Esoudiere
Copy link
Contributor Author

I guess I've done all that I could possibly see and nothing else is being pointed out to me..So I'll name it finished, in hope people will be more critical and point out everythin' else out to me.

@Esoudiere Esoudiere changed the title (Needs Review) Assembly System (Finished) Assembly System Mar 5, 2016
@Techhead0
Copy link
Contributor

For the record, it's been found that obj/item/weapon changes very little from obj/item.
See: https://github.com/Baystation12/Baystation12/blob/dev/code/game/objects/weapons.dm
And as far as I can tell, nothing at all is changed at the obj/item/device level.

@Esoudiere
Copy link
Contributor Author

Well, I personally put it under /device/ so that I could add /device/ to the Quick Create Object tool and easily spawn assemblies. Just a matter of categorisation. Plus future-proofin' and all that.

@Esoudiere
Copy link
Contributor Author

It cannot merge? :s

@Esoudiere
Copy link
Contributor Author

Apparently not, sorry about that. I'll have to fix it this afternoon.

  -Commented out health analyzer for now.
  -I copy pasted from the other scanners.dm without noticing it, but even so, how does the other one pass?
@Esoudiere
Copy link
Contributor Author

Fixed. I'll try to remember to pull every second day or so until this is either closed (which I assume is going to happen) or merged.

@comma comma removed the cannot merge label Mar 7, 2016
@GinjaNinja32
Copy link
Contributor

You're still hitting the "Sorry, we could not display the entire diff because it was too big." limit.

This PR is too big to effectively review; in its current state it is very unlikely to be merged. If you can split it into separate PRs that don't have cyclic dependencies it'd be far more likely to happen.

@PsiOmegaDelta
Copy link
Member

I might just lay some groundwork with an assembly/extension datum to make things easier.

@Esoudiere
Copy link
Contributor Author

I did split it into multiple PRs earlier, which is why the commit count is so high. But as I'm inexperienced with git, it was just a horrible experience I do not wish to repeat. In the final four (non-merge/bugfix) commits, you should be able to review everything hopefully? (Split into categories of Base Logic, Map Changes, Files not in the modules/assembly dir, Extras. The commit message should hopefully have enough detail to get the gist of the categories.) Everything before those four commits are from previous PRs.

I still don't fully understand how 'toying' with BYOND code to disinherit inheritance (I had to) is beneficial, I just thought it was a quicker way to do stuff not a more efficient way. But if you can indeed lay out some foundation, I'd be happy to try to add everything to it if you think it'd be better.

@mwerezak
Copy link
Contributor

mwerezak commented Mar 7, 2016

In the future, use git pull --rebase upstream/dev so that you don't end up with all of these merge nodes that complicate squashing old commits.

@Esoudiere
Copy link
Contributor Author

Should I just close this PR & /try/ making two different ones again? (Using the rebase feature to get rid of past commit nodes, thanks mwerezak) Or are the commits small enough, Ginja?

@GinjaNinja32
Copy link
Contributor

splitting it into commits doesn't make it any easier to review, it's still a seven-thousand-line PR even if it is in 37 commits.

I'm still not entirely sure how you got to 7000 lines.

@mwerezak
Copy link
Contributor

I'm not sure where you should start but perhaps consider making a PR with all new items removed, for example, and then creating a second PR for just new items. Or any other way you can split this PR.

@Esoudiere
Copy link
Contributor Author

7000 of the line changes are for new items, across about 32 files. Would that be too big for a single PR? (Also, my computer has broken down most unfortunately..So, might have to wait a while ;;)

@Esoudiere
Copy link
Contributor Author

Closing to split

@Esoudiere Esoudiere closed this Mar 15, 2016
@mwerezak mwerezak mentioned this pull request Mar 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants