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

Add .life serialization support to lsns #652

Merged
merged 11 commits into from Jun 25, 2019
Merged

Add .life serialization support to lsns #652

merged 11 commits into from Jun 25, 2019

Conversation

cracker1000
Copy link
Contributor

Add digitisation to LSNS

@jacob1 jacob1 changed the title Add digitisation support to lsns Add .life serialization support to lsns Jun 8, 2019
src/simulation/elements/LSNS.cpp Outdated Show resolved Hide resolved
if (!r)
continue;

if (deserializelife && TYP(r) == PT_FILT)
Copy link
Member

Choose a reason for hiding this comment

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

This if statement is wrong, so is the next one. This one sets the .life of the LSNS, which isn't supposed to happen (that would cause the LSNS to generate a spark). It should set the .life of the non-FILT particle using this ctype calculation.

In addition, I think it would be a good idea to restrict it to newlife > 0. I don't want people setting negative life. (Also, please actually use newlife instead of duplicating the ctype check twice)

Copy link
Member

Choose a reason for hiding this comment

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

I looked at it more, I didn't fully understand the problem before so my advice was wrong. The issue I have with this is the fact that there are two if statements and LSNS is modified. LSNS shouldn't be modified, and there should only be one if statement.

How it should work is, LSNS with .tmp = 2 scans around itself for a FILT. When it finds it, it then sets the .life of nearby particles. It should either: set the .life of the particle directly opposite the LSNS; or set the .life of all particles touching it. I don't really care which. Setting the .life of the opposite particle would be easier.

src/simulation/elements/LSNS.cpp Outdated Show resolved Hide resolved
@jacob1
Copy link
Member

jacob1 commented Jun 8, 2019

Made a few comments. I think using LSNS for both serialization and deserialization is a good idea, was going to suggest that :). Should hopefully be useful, allowing setting .life with LSNS.

@cracker1000
Copy link
Contributor Author

Thanks for pointing out these things, i will try to do suggested changes tomorrow.
Also the way i intended it to work was that, filt first sets the life of LSNS (with .tmp=2) and then this LSNS sets nearby non FILT part's life.

Prevent users from serializing negative .life values.
Allow LSNS to directly set nearby particle's life.
Change tmp values for modes to maintain consistency among various sensors.
Copy link
Member

@jacob1 jacob1 left a comment

Choose a reason for hiding this comment

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

Please don't duplicate the entire trigger loop. Just have a check in the middle for whether invert mode is activated.

Remove unnecessary checks.
Fix deserialization not working in certain directions
rewrite all the code to prevent conflicts.
Fix invert mode not working under certain conditions.
Copy link
Member

@LBPHacker LBPHacker left a comment

Choose a reason for hiding this comment

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

Just nitpicking, but it'd be nice if you considered these. It may be just me but I feel like the code would be a tiny bit more readable with these. Intentionally just "commenting" and not "requesting changes".

continue;
if (parts[ID(r)].life > parts[i].temp-273.15)
parts[i].life = 1;
if (parts[i].tmp == 1 && TYP(r) != PT_LSNS && TYP(r) != PT_FILT && parts[ID(r)].life >= 0)
Copy link
Member

Choose a reason for hiding this comment

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

Make decisions based on tmp with a switch? (case 1, case 2, case 3, default)

for (ry=-rd; ry<rd+1; ry++)
if (x+rx>=0 && y+ry>=0 && x+rx<XRES && y+ry<YRES && (rx || ry))
bool setFilt = false;
bool setFiltd = false;
Copy link
Member

Choose a reason for hiding this comment

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

Raname this to setLife?

setFilt = true;
photonWl = parts[ID(r)].life;
}
if (parts[i].tmp == 3 && TYP(r) != PT_LSNS && TYP(r) == PT_FILT)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (parts[i].tmp == 3 && TYP(r) != PT_LSNS && TYP(r) == PT_FILT)
if (parts[i].tmp == 3 && TYP(r) == PT_FILT)

@LBPHacker LBPHacker merged commit b2adbb5 into The-Powder-Toy:master Jun 25, 2019
@cracker1000 cracker1000 deleted the topkek branch July 1, 2019 05:22
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

3 participants