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

Added color map support for simulation renderer #10

Closed
wants to merge 0 commits into from

Conversation

favreau
Copy link
Member

@favreau favreau commented Jul 9, 2016

No description provided.


file.close();
return validParsing;
}
Copy link
Contributor

@eile eile Jul 9, 2016

Choose a reason for hiding this comment

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

lunchbox::loadAscii( lexis::LookupTable1D& )

Or, in other words, I strongly object to yet another implementation of color map handling.

Copy link
Member Author

Choose a reason for hiding this comment

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

2 things:

  • That's two new mandatory dependencies that I'd need to add in order to replace 20 lines of basic c++ code, you'll need to convince me.
  • The existing implementation of the LookupTable1D is very 'livre' specific and does not currently satisfy my needs: typically additional parameters such as light emission and ambient occlusion strengths, and of course more than 256 values to match those of the simulation. It's already implemented in my branch, but I am pushing things little by little to make the reviewing easier. So let's first decide on the event, and then I will use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's something I don't understand here. After the discussion the other day I thought we had agreed on trying to take the use cases related to colormaps as an attempt to have one real shared component (and here I'm referring to a full library solution, not just an event schema). When we talked the other day about the colormap in Brayns, I thought you just wanted to do a small experiment with hardcoded values to understand better how it could work in OSPRay, but now you want to merge in 300 lines of code with the full feature. I know you want to try things, but I don't understand why you want to merge into master our third implementation of the same thing instead of making the effort in converging into a common solution. With this approach, the convergence won't ever happen. Was any user pressing to get simulation mapping in Brayns?

Copy link
Member Author

@favreau favreau Jul 11, 2016

Choose a reason for hiding this comment

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

My undestanding of the common part is in the definition of the colormap, and how to generate it, not in the way it is implemented in the application itself. I am of course willing to define a common way to generate color maps, if of course this makes sense considering the features provided by the various renderers. The current implementation does not define the color map, it only takes a set of values and makes them available to the simulation renderer. In my opinion, the common part resides outside of the application, for example in the form of a python library that we could use in a notebook (just an idea). I understand you already have some code there that I will be happy to reuse.
Regarding the last question, there is indeed an urgent request for media production that requires this feature.

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