{"action":"created","issue":{"url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/issues/912","repository_url":"https://api.github.com/repos/imageworks/OpenShadingLanguage","labels_url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/issues/912/labels{/name}","comments_url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/issues/912/comments","events_url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/issues/912/events","html_url":"https://github.com/imageworks/OpenShadingLanguage/pull/912","id":355006331,"node_id":"MDExOlB1bGxSZXF1ZXN0MjExNjM1Mjg5","number":912,"title":"Fix cellnoise bug","user":{"login":"lgritz","id":504179,"node_id":"MDQ6VXNlcjUwNDE3OQ==","avatar_url":"https://avatars0.githubusercontent.com/u/504179?v=4","gravatar_id":"","url":"https://api.github.com/users/lgritz","html_url":"https://github.com/lgritz","followers_url":"https://api.github.com/users/lgritz/followers","following_url":"https://api.github.com/users/lgritz/following{/other_user}","gists_url":"https://api.github.com/users/lgritz/gists{/gist_id}","starred_url":"https://api.github.com/users/lgritz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lgritz/subscriptions","organizations_url":"https://api.github.com/users/lgritz/orgs","repos_url":"https://api.github.com/users/lgritz/repos","events_url":"https://api.github.com/users/lgritz/events{/privacy}","received_events_url":"https://api.github.com/users/lgritz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-08-29T05:48:48Z","updated_at":"2018-08-30T19:38:12Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/pulls/912","html_url":"https://github.com/imageworks/OpenShadingLanguage/pull/912","diff_url":"https://github.com/imageworks/OpenShadingLanguage/pull/912.diff","patch_url":"https://github.com/imageworks/OpenShadingLanguage/pull/912.patch"},"body":"WARNING: possible visible change in procedural patterns\r\n\r\nIt's expected that cellnoise(x) == cellnoise(floor(x)), but we found an\r\nugly bug wherein cellnoise internally used a helper function\r\nquick_floor, which is supposed to take the floor of a float argument and\r\nreturn its integer equivalent. But unfortunately it had a really odd\r\nerror: when x was an exact negative integer, it gave the value for the\r\nnext smaller integer.\r\n\r\n    quick_floor( 1.00) ==  1\r\n    quick_floor( 0.01) ==  0\r\n    quick_floor( 0.00) ==  0\r\n    quick_floor(-0.01) == -1\r\n    quick_floor(-0.99) == -1\r\n    quick_floor(-1.00) == -2    // WRONG!!! Should be -1\r\n    quick_floor(-1.01) == -2\r\n\r\nThis in turn caused cellnoise lookups of exact negative integer values\r\nto be incorrect lookups of the next lower-numbered cell, i.e.,\r\ncellnoise(i) having the same value as cellnoise(i-epsilon), but again,\r\nonly for i<0 and i==floor(i) exact integer values.\r\n\r\nNow, if your shader made a visual representation of callnoise(x,y), it\r\nwould look just fine, unless a particular x,y fell *exactly* on integer\r\ncoordinates, in which case it would be ever so slightly off (by being\r\nthe color of the square immedately to the left instead of to the right,\r\nas it should be, say).  Nobody would ever notice that.\r\n\r\nAlso, if your shader took the floor before passing to cellnoise, like:\r\ncellnoise(floor(x),floor(y)), it would also *look* correct, though the\r\nnegative values would be wrong, since cellnoise(-1,-1) would have\r\nreturned the value that should have been cellnoise(-2,-2), etc., but\r\nyou never would have noticed that.\r\n\r\nBut... there were some possible uses of cellnoise where it was behaving\r\nvery strangely, and the main place you would notice this is certain\r\ntypes of \"voronoi\" noises where you are lookup up cellnoise values while\r\nlooping over adjacent lattice points. (Thanks, Zap, for noticing this,\r\nand so sorry that there was a legit bug here!)\r\n\r\nIt was a very slight possible problem for perlin style noise as well,\r\nbut since those noises are \"continuous\" and defined by gradients, you\r\nreally, really, would not have known that the occasional pixel had noise\r\nresults that were ever so slightly off in its derivatives only at those\r\nisolated integer lattice points. I think no visible artifacts were\r\ndiscernable under real-world conditions.\r\n\r\nThe solution is to bite the bullet and replace the clever (but extremely\r\nminor) speedup for computing floor, with a legit floor call.  It's ever\r\nso slightly more expensive if you isolate the cost of just raw cellnoise\r\ncalls, but in the context of a real render I don't think you'll be able\r\nto measure it.\r\n\r\nWARNING: This may change the appearance of certain \"Voronoi\" patterns,\r\nor possibly other uses of cellnoise as random number sequences. It also\r\nchanges the appearance of \"Gabor\" noise, which internally also used the\r\nfaulty quick_floor in such a way that it affects the whole pattern.\r\n\r\nNote that this should NOT change the appearance of any of the\r\nperlin-style noises, and you may not notice any change for most ordinary\r\nuses of cellnoise, either.\r\n\r\nI changed the oslnoise unit tests to check that this is working\r\nproperly, and updated all of the noise unit tests that generate images\r\nto \"center\" the patterns so that they visibly display the negative\r\nregions of the noise domain (they had all shown only positive domain,\r\nwhich is probably inviting trouble if there was ever a noise bug that\r\nonly affects what happens for negative values or right on the axes).\r\nThis caused a lot of the reference images to change, even for the tests\r\nof noise patterns that were unaffected. Also, I took the chance to merge\r\na few of the signed/unsigned tests for less repetition.\r\n\r\nI'm very sorry about this; changing the appearance of noise functions is\r\nnot something we take lightly, we do it only as a last resort. It was\r\nclearly wrong before and had some very unexpected failure modes. But I'm\r\nonly making this fix in master, which is destined to be \"OSL 2.0\", so I\r\nfigure it's as good a time as any to make a change that could possibly\r\nmake visible changes in some shaders.\r\n\r\nIf you see this as some kind of major betrayal that will screw up your\r\nproductions, let me know and if pressed I can perhaps make some kind of\r\nemulation of the old buggy behavior for people who need to preserve it.\r\nBut I'd rather not do that if it's not absolutely necessary.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/issues/comments/417442032","html_url":"https://github.com/imageworks/OpenShadingLanguage/pull/912#issuecomment-417442032","issue_url":"https://api.github.com/repos/imageworks/OpenShadingLanguage/issues/912","id":417442032,"node_id":"MDEyOklzc3VlQ29tbWVudDQxNzQ0MjAzMg==","user":{"login":"lgritz","id":504179,"node_id":"MDQ6VXNlcjUwNDE3OQ==","avatar_url":"https://avatars0.githubusercontent.com/u/504179?v=4","gravatar_id":"","url":"https://api.github.com/users/lgritz","html_url":"https://github.com/lgritz","followers_url":"https://api.github.com/users/lgritz/followers","following_url":"https://api.github.com/users/lgritz/following{/other_user}","gists_url":"https://api.github.com/users/lgritz/gists{/gist_id}","starred_url":"https://api.github.com/users/lgritz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lgritz/subscriptions","organizations_url":"https://api.github.com/users/lgritz/orgs","repos_url":"https://api.github.com/users/lgritz/repos","events_url":"https://api.github.com/users/lgritz/events{/privacy}","received_events_url":"https://api.github.com/users/lgritz/received_events","type":"User","site_admin":false},"created_at":"2018-08-30T19:38:12Z","updated_at":"2018-08-30T19:38:12Z","author_association":"MEMBER","body":"Hmmm... I guess the other potential reason to leave it as it was is if somebody chimes in later with a \"holy crap, I need the old pattern\", it would make it very easy to have a build-time switch that restores the old behavior. Not so easy once everything is changed to ifloor."}}
{"actor":{"display_login":"lgritz"}}