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

disable x3dom gamma correction ? #122

Open
andreasplesch opened this issue Nov 4, 2019 · 6 comments

Comments

@jamesleesaunders

This comment has been minimized.

Copy link
Owner

@jamesleesaunders jamesleesaunders commented Nov 4, 2019

Thanks @andreasplesch ! This was something I have been meaning to look into, I noticed the colour saturation difference when x3dom moved to 1.8.

Did you want to put in a PR with your fixes?

@andreasplesch

This comment has been minimized.

Copy link
Contributor Author

@andreasplesch andreasplesch commented Nov 4, 2019

x3dom uses a non-standard Environment node for these settings which means that x_ite generates a warning in the console about not recognizing such a node. Would you still want to go ahead ?

Also, I noticed there is a lot of code duplication for setting up the scene for each chart (making a x3d, scene and background element for each chart). I can insert an Environment node for each chart separately but it may start make sense to centralize and share utility functions between charts. I would not know how to do that best given the code structure.

@jamesleesaunders

This comment has been minimized.

Copy link
Owner

@jamesleesaunders jamesleesaunders commented Nov 4, 2019

I have just pulled the changes over from your gammaCorrection branch and merged them in.
Rather than modify the 'dist' files directly (which are built by npm) I have translated your changes into the src/charts files (699d68a) and rebuilt the dist files again.

As you point out there is a lot of code duplication where each of the src/chart/* files each has an identical createBase() function which created the x3d base, background (and now environment) tags. This createBase() does indeed need moving to a DRYer base module type place.

x3dom uses a non-standard Environment node for these settings which means that x_ite generates a warning in the console about not recognizing such a node. Would you still want to go ahead ?

ah, I have already merged in your changes, I'll have a look at how this manifests itself for x_ite and make may have to implement something which detects whether it is running on x3dom or x_ite.
I do want to try and support x3dom and x_ite equally and ideally I don't want to drift too far off standard, do you know the reason why this was switched between x3dom 1.7.2 and 1.8 ?

@jamesleesaunders

This comment has been minimized.

Copy link
Owner

@jamesleesaunders jamesleesaunders commented Nov 4, 2019

huurm, yes,
x_ite.min.js:19 XML Parser Error: Unkown node type 'ENVIRONMENT', you probably have insufficient component/profile statements.
Not the end of the world, but I'll have a think whether to revert this for the moment.

@andreasplesch

This comment has been minimized.

Copy link
Contributor Author

@andreasplesch andreasplesch commented Nov 4, 2019

No, I do not know why gamma correction is linear by default. Presumably, it is considered more correct and more standard.

@andreasplesch

This comment has been minimized.

Copy link
Contributor Author

@andreasplesch andreasplesch commented Nov 5, 2019

Another way to switch to no gamma correction by default would be to patch up x3dom after loading.

Here is one way to do that:

andreasplesch@955eb9e

https://raw.githack.com/andreasplesch/d3-x3d/x3domGamma/examples/X3DOM/chart/ScatterPlot.html

x_ite does not show warnings this way:
https://raw.githack.com/andreasplesch/d3-x3d/x3domGamma/examples/X_ITE/chart/ScatterPlot.html

But it could be considered hackish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.