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

bumpy flatmap fixes #310

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

bumpy flatmap fixes #310

wants to merge 2 commits into from

Conversation

alexhuth
Copy link
Contributor

To (~) productively procrastinate from writing, I fixed some issues with the bumpy flatmap rendering. First let me remind y'all what the point of bumpy flatmaps is.

FLATMAPS ARE HARD TO READ. People (current audience excluded) often have no fuuucking clue what they're looking at on a flatmap, because flatmaps are typically missing landmarks. This is especially bad for flatmaps with un-thresholded data, because then the typical binary curvature map (which can help orient folks) is missing as well. We try to alleviate these problems by adding ROI outlines and lines indicating sulci & gyri. But I don't think that's sufficient.

The goal of bumpy flatmaps is to give subtle visual cues (through shading and, if you're into that kind of thing, specularity) that show where sulci and gyri are. I think it works kinda well?

The changes in this branch:

  1. I fixed a bug where the bump height was being smoothed in the right hemisphere but not the left. I also reduced the amount of smoothing somewhat. The amount of smoothing can be tweaked if it's currently too rough for peoples' preferences.

  2. I tweaked the lighting (which affects not only the flatmap but other views as well) to follow the top-left lighting convention for topographic maps, which imo makes it much more obvious that sulci are "valleys" and gyri are "mountains". This also had the effect of making the lighting dimmer overall, which is not ideal but we can fix.

An example:

Before these changes
screen shot 2019-01-25 at 9 48 14 am

After these changes
screen shot 2019-01-25 at 9 48 21 am

@sslivkoff
Copy link
Contributor

screenshot of the new changes looks pretty nice. could you post one with actual data on it? e.g. rgb semantic pcs

flatmaps are very very difficult to build an intuition for. I strongly agree that we need to continue innovating in that area

@marklescroart
Copy link
Contributor

marklescroart commented Jan 25, 2019

I suspect much of that justification was aimed at me, since I have vocally objected to specularity and bumps. Fair points, I will stop complaining, this is reasonably useful (but I will still leave them off by default). I would also prefer to address the dimness issue before merging (does it only affect the lighting with the bumps on? or all the time? if all the time, then I strongly prefer fixing before merge.) Also - any chance that while you're procrastinating you could address #286 ?? (also a lighting issue in there)

Base automatically changed from master to main January 19, 2021 20:02
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.

3 participants