Skip to content
This repository was archived by the owner on May 22, 2025. It is now read-only.

Conversation

@Simplehorror
Copy link
Contributor

Document the changes in your pull request

guessing that priority doesn't work since they sort into sublists but they're actually only sorted inside the sublists which doesn't make sense when you start pulling them out of these sublists, you would need to sort them again.

Spriting

Wiki Documentation

Changelog

🆑
bugfix: Sort gas reactions by priority properly
/:cl:

@Yogbot-13 Yogbot-13 added the Fix This fixes an issue. Please link issues in fix PRs label Dec 1, 2022
@Simplehorror
Copy link
Contributor Author

Simplehorror commented Dec 1, 2022

just checked upstream and found this (which confirms my fix is correct)
tgstation/tgstation#48871
amazing how it takes me 10 seconds to fix this when I'm aware of it, maybe file a bug report next time @SapphicOverload?

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

Test this and make sure hyper noblium doesn't just break the entire gas reaction loop after running 1 time since that is the other possible issue rather then this just being an error with sorting.

@Simplehorror
Copy link
Contributor Author

Test this and make sure hyper noblium doesn't just break the entire gas reaction loop after running 1 time since that is the other possible issue rather then this just being an error with sorting.

it's not that

@warface1234455
Copy link
Contributor

I will test this for him

@warface1234455
Copy link
Contributor

just checked upstream and found this (which confirms my fix is correct) tgstation/tgstation#48871 amazing how it takes me 10 seconds to fix this when I'm aware of it, maybe file a bug report next time @SapphicOverload?

didnt know there was a priority fix on tg damn

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

Also just a code side of things was this not possible using the previous proc instead of creating an entirely new proc?

@warface1234455
Copy link
Contributor

warface1234455 commented Dec 1, 2022

@Simplehorror [09:17:02] Runtime in reactions.dm,62: Cannot read null.priority

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

@Simplehorror REASON #8: [09:17:02] Runtime in reactions.dm,62: Cannot read null.priority

It ran into an error with his new proc which is why the old proc was coded the way it was.

@warface1234455
Copy link
Contributor

@Simplehorror REASON #8: [09:17:02] Runtime in reactions.dm,62: Cannot read null.priority

It ran into an error with his new proc which is why the old proc was coded the way it was.

yeah

@warface1234455
Copy link
Contributor

the tg fix deleted the old proc

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

yeah

In theory though if that is the ONLY issue causing noblium to not work you can just use the old proc to create the list in the new location, problem is this means you are now running far more procs per gas reaction going on.

@Simplehorror
Copy link
Contributor Author

@Simplehorror REASON #8: [09:17:02] Runtime in reactions.dm,62: Cannot read null.priority

It ran into an error with his new proc which is why the old proc was coded the way it was.

what? these are different procs

@Simplehorror
Copy link
Contributor Author

@Simplehorror [09:17:02] Runtime in reactions.dm,62: Cannot read null.priority

yeah give me a bit im working on grues right now, my cmp function is being fed nulls

@warface1234455
Copy link
Contributor

didnt see the s there

@Simplehorror
Copy link
Contributor Author

ok should be fixed, it was trying to sort an associative list and it isn't

@warface1234455
Copy link
Contributor

warface1234455 commented Dec 1, 2022

ok should be fixed, it was trying to sort an associative list and it isn't

ok imma gonna run a debug server and test

@Simplehorror
Copy link
Contributor Author

also I appreciate rdrazga shittalking me in the discord for fixing an atmos bug because it didn't work first try

@warface1234455
Copy link
Contributor

also I appreciate rdrazga shittalking me in the discord for fixing an atmos bug because it didn't work first try

so you are actually in discord

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

also I appreciate rdrazga shittalking me in the discord for fixing an atmos bug because it didn't work first try

Mate you are posting OPEN PRs that you are not even taking 2 minutes to bug test of course I am going to point it out.

@Simplehorror
Copy link
Contributor Author

yes because i am working on grues and nobody else was going to do it if i didnt, be happy it happened

@Simplehorror
Copy link
Contributor Author

obviously the code didn't work (first try), but the logic i had in mind was correct

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

Ok fair I won't shittalk you as long as you do atleast spend time to bug and lag test this compared to the old code before getting it merged. If this breaks the server it won't be a good fix, if this lags the server people would much rather deal with not having hyper noblium forming properly rather then just laggy atmos reactions. This was a problem with the original TG fix it looks like so it should be a consideration.

@Simplehorror
Copy link
Contributor Author

Simplehorror commented Dec 1, 2022

it's not going to lag the server, we don't use LINDA + a 5 list sort is nothing, plus I hope being respected isn't being conditional

Copy link
Contributor

@warface1234455 warface1234455 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
alright seems to work as intended, no lag, gas reactions are pretty smooth

@warface1234455
Copy link
Contributor

the middle chamber is hypernob production, the right one is fusion chamber which is being stopped by hypernob

@warface1234455
Copy link
Contributor

this is the first time im asking for a speedmerge of simeplehorror's PR

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

In other news for the sake of my sanity (and far more likely future coders sanity) I am going to figure out how to github and put comments on all this dang code to explain what the hell is actually happening in half of this stuff because Jesus.

@Rdrazga
Copy link

Rdrazga commented Dec 1, 2022

this is the first time im asking for a speedmerge of simeplehorror's PR

NGL I don't think hyper noblium forming is that pressing of an issue, also kat is going to yell at us for conflicting auxmos again

@warface1234455
Copy link
Contributor

warface1234455 commented Dec 1, 2022

this is the first time im asking for a speedmerge of simeplehorror's PR

NGL I don't think hyper noblium forming is that pressing of an issue, also kat is going to yell at us for conflicting auxmos again

he will get it sorted out no worries, im atmos mains so i am on my knees for this

@Simplehorror
Copy link
Contributor Author

In other news for the sake of my sanity (and far more likely future coders sanity) I am going to figure out how to github and put comments on all this dang code to explain what the hell is actually happening in half of this stuff because Jesus.

i fear that you will leave incorrect comments (which would be worse if I read comments)

@SapphicOverload
Copy link
Contributor

works fine but hotspots from igniters and such still show up even when there's no reaction happening
image

@Simplehorror
Copy link
Contributor Author

works fine but hotspots from igniters and such still show up even when there's no reaction happening image

yeah igniters will still generate hotspot, but the hotspot wont react with anything, it's not a bug

@SapphicOverload
Copy link
Contributor

yeah igniters will still generate hotspot, but the hotspot wont react with anything, it's not a bug

it can raise the temperature as it if's burning when it's not actually burning so i recommend fixing it

@Simplehorror
Copy link
Contributor Author

that is what an igniter is going to do

@SapphicOverload
Copy link
Contributor

that is what an igniter is going to do

it only heats the gas mixture if there's flammable gases in it, because the point of igniters raising temperature is to get the reaction to start, which shouldn't be happening if there's hypernoblium. it's not intended to be a heater.

@SapphicOverload
Copy link
Contributor

SapphicOverload commented Dec 1, 2022

alright i've made my own pr for that (#16893), but i can close it if you want to include the changes in yours

Copy link
Contributor

@ReddicusDragon ReddicusDragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not even going to try reading atmos code it's black magic

@Simplehorror
Copy link
Contributor Author

I'm not even going to try reading atmos code it's black magic

no it is not

Copy link
Contributor

@MajManatee MajManatee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test your own god damned code in the future, or im just gonna start closing your PRs

@Simplehorror
Copy link
Contributor Author

i did test this

@Bibby0110
Copy link
Contributor

@monster860

@warface1234455
Copy link
Contributor

@monster860

github ping is shit

@Simplehorror
Copy link
Contributor Author

remind me to do this properly

@warface1234455
Copy link
Contributor

warface1234455 commented Dec 16, 2022

remind me to do this properly

What else

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Fix This fixes an issue. Please link issues in fix PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants