-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/low affinity values #186
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
Conversation
…into feature/low-affinity-values
src/App.tsx
Outdated
| if ( | ||
| section === Section.Experiment && | ||
| timeFactor !== LiveSimulationData.DEFAULT_TIME_FACTOR | ||
| ) { | ||
| setTimeFactor(LiveSimulationData.DEFAULT_TIME_FACTOR); | ||
| } else if (section === Section.Introduction) { | ||
| } else if ( | ||
| section === Section.Introduction && | ||
| timeFactor !== LiveSimulationData.INITIAL_TIME_FACTOR | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes aren't directly connected, but I did realize i was doing more updates to state than I needed to, which became more of an issue when i'm pushing the rendering
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could alternatively pull the const { section } = content[currentModule][page] out of the useEffect and have the useEffect only trigger on changes to it?
| min={min} | ||
| max={max} | ||
| step={2} | ||
| step={stepSize.current} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the second module has a much bigger range, so we're showing bigger jumps between markers
| this.currentNumberOfUnbindingEvents = 0; | ||
| } | ||
|
|
||
| private resolveChildPositions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the big change: before I was using the "isTrigger" flag to keep the detect-collisions library from doing the overlap check if the spheres were bound, and then doing my own collision detection of the complex. But now, I'm just manually moving the child back in place after the system has resolved. This means the bound ligands can sometimes overlap each other but it's not super serious, and better than how it looked before
ShrimpCryptid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some questions/suggestions!
| const agentId = LiveSimulationData.AVAILABLE_AGENTS[agentName].id; | ||
| clientSimulator.changeConcentration(agentId, value); | ||
| simulariumController.gotoTime(time + 1); | ||
| simulariumController.gotoTime(1); // the number isn't used, but it triggers the update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In an ideal world, what would the API for this look like? Something like update() or advance() for simulations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way I was doing it before may be more ideal (ie "give me the next time step"). the issue was because of the async nature of the connection between the app and the viewer, sometimes what the app thought was the next frame, the viewer thought was the current frame, so it wouldn't update.
It might be nice to be able to pass something like "++1" to gotoTime meaning "whatever you think is the next frame, give it to me".
src/App.tsx
Outdated
| if ( | ||
| section === Section.Experiment && | ||
| timeFactor !== LiveSimulationData.DEFAULT_TIME_FACTOR | ||
| ) { | ||
| setTimeFactor(LiveSimulationData.DEFAULT_TIME_FACTOR); | ||
| } else if (section === Section.Introduction) { | ||
| } else if ( | ||
| section === Section.Introduction && | ||
| timeFactor !== LiveSimulationData.INITIAL_TIME_FACTOR | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could alternatively pull the const { section } = content[currentModule][page] out of the useEffect and have the useEffect only trigger on changes to it?
| max={100} | ||
| step={1} | ||
| initialValue={timeFactor} | ||
| overrideValue={timeFactor} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does overrideValue here do? Just keeping it synced with timeFactor until a user interacts with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's basically "value". it's because Slider itself is a wrapper on the Antd component that already stores it's own value on update
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const disabledNumbers = [0]; | ||
|
|
||
| const stepSize = useRef(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just be, const stepSize = (max - min) / 5 instead of a ref? It looks like the useMemo updates whenever max and min do anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this; alternately, could become its own useMemo, or be returned from the useMemo for marks below in a 2-tuple or object
| const getSuccessMessage = (selectedAnswer: number) => { | ||
| if (selectedAnswer < 5) { | ||
| return ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: function could live outside component body
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const disabledNumbers = [0]; | ||
|
|
||
| const stepSize = useRef(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this; alternately, could become its own useMemo, or be returned from the useMemo for marks below in a 2-tuple or object
Co-authored-by: Cameron Fraser <[email protected]>
Co-authored-by: Cameron Fraser <[email protected]>
Problem
Estimated review size: md
The app needs to compensate for how different the values in module 2 are from module 1.
closes #177
closes #178
Solution
Updated a few more hard coded items.
The low affinity module also need MANY more spheres to be rendered, so I did some more testing on the collision checks and was able to simply things to make it more performant and also has less overlaps.
Type of change
Please delete options that are not relevant.
Steps to Verify:
Screenshots (optional):
bound ligands have a more consistent position relative to the parent:
