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

fix handling of flags 1-3 in coons shading #6304

Merged
merged 1 commit into from
Aug 3, 2015

Conversation

CodingFabian
Copy link
Contributor

Short story: somebody got lost in two different indices. pi is an index in the stream and is explained on page 198 of the 32000-spec (however 1-based there), and ps is an index to something in PDF.js. I used the code from flag 0 (which works) to understand which is which. It is also important to understand that for flags 1,2 and 3, the stream is always assigned to the same coordinates and colors. What changes is which "old" coordinates and colors are assigned to what is "missing" in the stream. This is why for these flags, the code is identical except for the assignments in the first "row".

Largely fixes #4227.

"file": "pdfs/coons-allflags-withfunction.pdf",
"md5": "c5f79c24bf9eb66698be0e4ecaa1bdf8",
"rounds": 1,
"type": "eq",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra comma (bad in JSON)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the indenting is a bit off. Compare with the ones below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

huh? shows identical in my editor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah. lol. perhaps i copied the only other bad one in this file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually the whole file is an indentation mess

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Keep it like https://github.com/CodingFabian/pdf.js/blob/fixup-6303/test/test_manifest.json#L2331-L2336 for this PR as that is what we always use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can fixup the indention in this file, but which one is the wanted one. give me a reference test case and ill do it now

Copy link
Contributor

Choose a reason for hiding this comment

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

We should do that in a different PR. I actually do not like them all myself, and I would prefer:

[
  {
    "id": "foo",
    "name": "bar"
  }
]

which is also how JSONLint formats it (and a lot of other projects). But for this PR the current commit is fine!

Short story: somebody got lost in two different indices. pi is an index in the stream and is explained on page 198 of the 32000-spec (however 1-based there), and ps is an index to something in PDF.js. I used the code from flag 0 (which works) to understand which is which. It is also important to understand that for flags 1,2 and 3, the stream is always assigned to the same coordinates and colors. What changes is which "old" coordinates and colors are assigned to what is "missing" in the stream. This is why for these flags, the code is identical except for the assignments in the first "row".
@timvandermeij
Copy link
Contributor

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Linux)


Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/8dfb83cba8948b8/output.txt

@timvandermeij
Copy link
Contributor

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Windows)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/f0369cfc15252f4/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Linux)


Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/19e78e672f8d8de/output.txt

@THausherr
Copy link
Contributor

looks much better now (but there is still work to do to make your sample render as it should :-))

Now I see what you mean (I had tested my patch on the CIB file, not on the file I created today). This looks like the effect that happens also with the lamp_cairo.pdf file. (I have a patch for type 7, and it works fine for another test file, but not for lamp_cairo.pdf). I have the suspicion that this is related to the effect of "extra stuff" that has been seen with types 4 and 5, see #6294.

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/f0369cfc15252f4/output.txt

Total script time: 18.37 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/19e78e672f8d8de/output.txt

Total script time: 18.97 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: Passed

yurydelendik added a commit that referenced this pull request Aug 3, 2015
fix handling of flags 1-3 in coons shading
@yurydelendik yurydelendik merged commit 1da7e89 into mozilla:master Aug 3, 2015
@yurydelendik
Copy link
Contributor

Thank you for the patch.

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Linux)


Received

Command cmd_makeref from @yurydelendik received. Current queue size: 0

Live output at: http://107.21.233.14:8877/1cbe59bb183f311/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Windows)


Received

Command cmd_makeref from @yurydelendik received. Current queue size: 0

Live output at: http://107.22.172.223:8877/471718ec3d85828/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Windows)


Success

Full output at http://107.22.172.223:8877/471718ec3d85828/output.txt

Total script time: 18.55 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

pdfjsbot commented Aug 3, 2015

From: Bot.io (Linux)


Success

Full output at http://107.21.233.14:8877/1cbe59bb183f311/output.txt

Total script time: 18.76 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

THausherr added a commit to THausherr/pdf.js that referenced this pull request Aug 4, 2015
pi is an index in the stream and is explained on page 201 of the 32000-spec (however 1-based there), and ps is an index to something in PDF.js. I used the code from flag 0 (which works) to understand which is which. It is also important to understand that for flags 1,2 and 3, the stream is always assigned to the same coordinates and colors. What changes is which "old" coordinates and colors are assigned to what is "missing" in the stream. This is why for these flags, the code is identical except for the assignments in the first "row". (Same principle as in mozilla#6304). Note that this change will not improve the lamp_cairo.pdf file, only the two files mentioned in mozilla#6305.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Coons patch mesh gradient displays white areas
5 participants