Yogstation icon indicating copy to clipboard operation
Yogstation copied to clipboard

Update to signals, defines, and component argslist

Open Chubbygummibear opened this issue 2 years ago • 10 comments

Document the changes in your pull request

ports: https://github.com/tgstation/tgstation/pull/63222 https://github.com/tgstation/tgstation/pull/62970 https://github.com/tgstation/tgstation/pull/65809 https://github.com/tgstation/tgstation/pull/59185

however like none of the new signals or defines are actually implemented so nothing really changes as a result of this pr, this is just like modernizing our tools

we can move to implementing some of these after having a modernized backend that's more reflective of tg's (and the current year)

does this work?

it compiles and i started a game with the changes and was able to walk around with no errors. hard to tell if something deeper is broke

Wiki Documentation

i want wiki to make a page for each define (this is a joke)

Changelog

:cl:
rscadd: components can take named argslists rscadd: lots more signals and defines tweak: renames a few defines used throughout the codebase, no overall changes spellcheck: dextery /:cl:

Chubbygummibear avatar Oct 15 '22 20:10 Chubbygummibear

awooga

ToasterBiome avatar Oct 15 '22 20:10 ToasterBiome

You don’t need that long a changelog

TheGamerdk avatar Oct 15 '22 20:10 TheGamerdk

At first glance this seems counterproductive since it means we won’t get compile time errors for unimplemented signals. If people port TG related code they might not realize that the signal they rely on isn’t actually implemented.

TheGamerdk avatar Oct 15 '22 20:10 TheGamerdk

changelog should preferably only contain player facing changes but this is fine I guess

TheGamerdk avatar Oct 15 '22 20:10 TheGamerdk

At first glance this seems counterproductive since it means we won’t get compile time errors for unimplemented signals. If people port TG related code they might not realize that the signal they rely on isn’t actually implemented.

true that this doesn't initially do much, but the file structure reformatting as well as some of the macro wrappers being updated is essential for what i was trying to do with porting elements, and helps by cutting down the overall number of files changed for that pr to work since i was going on about 300 files changed when i stopped to do this part on its own

Chubbygummibear avatar Oct 15 '22 21:10 Chubbygummibear

At first glance this seems counterproductive since it means we won’t get compile time errors for unimplemented signals. If people port TG related code they might not realize that the signal they rely on isn’t actually implemented.

true that this doesn't initially do much, but the file structure reformatting as well as some of the macro wrappers being updated is essential for what i was trying to do with porting elements, and helps by cutting down the overall number of files changed for that pr to work since i was going on about 300 files changed when i stopped to do this part on its own

It doesn’t just “initially not do much”, it’s going to cause annoying to fix bugs in the future. The helpful aspect is the renaming of signals that we already have, the changes to backend code and similar. Preferably every new COMSIG define that isn’t used was removed.

TheGamerdk avatar Oct 16 '22 06:10 TheGamerdk

At first glance this seems counterproductive since it means we won’t get compile time errors for unimplemented signals. If people port TG related code they might not realize that the signal they rely on isn’t actually implemented.

true that this doesn't initially do much, but the file structure reformatting as well as some of the macro wrappers being updated is essential for what i was trying to do with porting elements, and helps by cutting down the overall number of files changed for that pr to work since i was going on about 300 files changed when i stopped to do this part on its own

It doesn’t just “initially not do much”, it’s going to cause annoying to fix bugs in the future. The helpful aspect is the renaming of signals that we already have, the changes to backend code and similar. Preferably every new COMSIG define that isn’t used was removed.

So this PR was born of the fact that as I was trying to do the elements update, I effectively had to do the signals and defines update to make it functional. I decided to split this off into its own PR because that branch is currently on about 300 files changed and 16k line changes. I plan to start again without the signals and defines following this. However if you would rather me try to implement as many as I can. I can try this on this PR to prevent confusion as to which are and are not used. My fear was having one PR that gets too large to be reviewed cleanly, but considering we're pretty much already there, I can just continue from here. As far as bugs going forward, I don't believe many will be born of this as it's functionally identical to what we currently have, but expanded and organized to match tg's so that porting is easier to navigate in the future and doesn't end up with a slow piecemeal update over the course of years. If it gets updated like this in one go, the need for future changes to defines will be cut down dramatically.

Chubbygummibear avatar Oct 16 '22 22:10 Chubbygummibear

At first glance this seems counterproductive since it means we won’t get compile time errors for unimplemented signals. If people port TG related code they might not realize that the signal they rely on isn’t actually implemented.

true that this doesn't initially do much, but the file structure reformatting as well as some of the macro wrappers being updated is essential for what i was trying to do with porting elements, and helps by cutting down the overall number of files changed for that pr to work since i was going on about 300 files changed when i stopped to do this part on its own

It doesn’t just “initially not do much”, it’s going to cause annoying to fix bugs in the future. The helpful aspect is the renaming of signals that we already have, the changes to backend code and similar. Preferably every new COMSIG define that isn’t used was removed.

So this PR was born of the fact that as I was trying to do the elements update, I effectively had to do the signals and defines update to make it functional. I decided to split this off into its own PR because that branch is currently on about 300 files changed and 16k line changes. I plan to start again without the signals and defines following this. However if you would rather me try to implement as many as I can. I can try this on this PR to prevent confusion as to which are and are not used. My fear was having one PR that gets too large to be reviewed cleanly, but considering we're pretty much already there, I can just continue from here. As far as bugs going forward, I don't believe many will be born of this as it's functionally identical to what we currently have, but expanded and organized to match tg's so that porting is easier to navigate in the future and doesn't end up with a slow piecemeal update over the course of years. If it gets updated like this in one go, the need for future changes to defines will be cut down dramatically.

We’re already at a stage where this is too large to review. The bugs aren’t going to be caused by this code, but as a result of this code.

Currently when you port a TG pr and there’s an undefined signal you know that you’ll have to implement it. With this PR no error would occur, the feature just wouldn’t work.

Either remove the unused signals or implement all of the places where they’re sent. Or implement some of them and delete the ones you don’t implement.

TheGamerdk avatar Oct 17 '22 08:10 TheGamerdk

bet

Chubbygummibear avatar Oct 17 '22 09:10 Chubbygummibear

bet

can’t wait to see it done :)

TheGamerdk avatar Oct 18 '22 19:10 TheGamerdk

You are changing values as well as modernising it, you are mucking with the MC which im afraid will cause more issues.

JamieD1 avatar Oct 29 '22 13:10 JamieD1

You are changing values as well as modernising it, you are mucking with the MC which im afraid will cause more issues.

Worth a shot to have this done at least, I think the benefits outweigh any problems we will come across with this PR. Problems are always fixable.

ToasterBiome avatar Oct 29 '22 21:10 ToasterBiome

You are changing values as well as modernising it, you are mucking with the MC which im afraid will cause more issues.

this is my 2nd attempt at doing backend updates, because the first time was going to be elements and components updates, which I stopped working on when I realized that in order to facilitate elements, I needed far more defines and signals than we had, so I started adding those in that PR as well. However it quickly got massively overwhelming in the number of changes. So I stopped with the intent of atomizing it by starting with updating JUST the defines, which is what this PR was. However, as Bibby said prior, he believes adding just the defines without implementations would lead to more issues, so at his behest I started implementing them as well.

The issue with implementing the new signals is that much of tg's backend is vastly different from ours. So if I were to try and implement the signals, I would have to work backwards to make an older version that works with us, and then eventually when we update more, like in the case of how TG moved all their obj defines down to the atom level defines. All those signals would have to be rewritten AGAIN.

I don't intend to try and do a massively elaborate backend update for a 3rd time. So considering that one of the largest bounties on the yog forums is asking for help in updating to TG backend standards, the options I see are:

You let me finish the changes and we try to work through the issues with the help of you and maintainers and whoever else we can drag in to work out the kinks or You tell me to stop, and we wait for the next person who wants to work on backend updates instead of making new feature PRs

Chubbygummibear avatar Oct 30 '22 22:10 Chubbygummibear

de stale und de conflict

TheGamerdk avatar Nov 21 '22 23:11 TheGamerdk