When there is original variations in the sgf file, it is not displayed at the correct position

xcombelle commented 9 years ago

Looks that the variations appear nearly randomly as A B C ....

for example in this file: https://gokibitz.com/kifu/VJdTzp84

The variations position appear not at the right node

neagle commented 9 years ago

Thanks for the bug report! This one worries me a little bit, as variations in SGFs are handled by the WGo.js player, and I've never had any trouble with them before.

Could you help me a little bit? Could you link to the move that shows incorrect variation labels / positions? Then I can compare against another SGF viewer (like cGoban or Eidogo) and try to look more deeply into the problem.

sphaso commented 9 years ago

Heya, this is a game of mine (Lumo) that was uploaded by my sensei. Maybe I can help with a few more info:

this is a very apparent bug: https://gokibitz.com/kifu/VJdTzp84?path=12 if you click on "B" white plays... twice?!
I initially commented it using cGoban, sent it to Ben via email, I'm not sure what editor he used [EDIT: by looking at the SGF I can tell he used MultiGo] but I asked him to give me the SGF file before he uploaded it (here: https://www.dropbox.com/s/rclsn6dyu7ettv4/execration-lumoReCommented.sgf?dl=0)
if you compare some branches in cGoban and GoKibitz it appears as if wGo is not displaying them all (don't have a precise reference right now, bit in a hurry, sorry)

sphaso commented 9 years ago

About point 1 (white playing twice): what's even more peculiar is that in the original SGF there's no variation on that coordinate, it would be at E6 instead. Another example at move 57, the B variation should be the cut at R7! About point 2: still cannot produce a smaller example SGF :\ About point 3: nevermind, I might have seen ghosts

neagle commented 9 years ago

Thanks a bunch for the details. It looks like the SGF is malformed, though I'm still tracking down exactly how. This SGF validator throws a critical warning about a parenthesis, which would probably be enough to throw off WGo.js's SGF parser.

sphaso commented 9 years ago

What switches are you using with the SGF Validator? the default settings are just giving me a warning about "MULTIGO":

Options: -c -b1 -l1 -p -e -t

Line:3 Col:34 - Warning 31: property identifier consists of more than 2 uppercase letters: <MULTIGOGM>
Line:3 Col:25 - Warning 35: unknown property <MULTIGOGM> found
/tmp/29713-4399.sgf: 2 warning(s)

I removed the MULTIGOGM property and it still shows wrong on GoKibitz.

It's also weird that cGoban can read it just fine :\

neagle commented 9 years ago

I didn't change any of the default settings. I'm using the content of the file from GoKibitz.

Options: -c -b1 -l1 -p -e -t

Line:1 Col:204 - Warning 31: property identifier consists of more than 2 uppercase letters: <MULTIGOGM>
Line:1 Col:195 - Warning 35: unknown property <MULTIGOGM> found
Line:99 Col:79 - Error 8 (critical): illegal char(s) found: ")"
/tmp/4037-376.sgf: 1 error(s)  2 warning(s)  (critical:1)

It's not that weird that cGoban might parse the file just fine and WGo might not. Everybody in the world—including me reimplements an SGF parser when they make their project. Why? Because we're all crazy masochists, I guess, and writing an SGF parser is just hard enough that it's really fun and just easy enough that it's still really fun. And people's parsers use strikingly different approaches.

Compare:

I can't check out cGoban's since it's not open-source.

My hunch, honestly, is that there's something about WGo's comparatively regex-reliant approach that is more fragile than others, but I still have to actually track down the cause of the error, which is proving elusive.

sphaso commented 9 years ago

I found where wGo is not showing one variation! Look at move 13, there should be another variation at C4. I wasn't hallucinating afterall :) What puzzles me now is how the file downloaded from GoKibitz is different from the original file that was uploaded. Does wGo \ GoKibitz massage the SGF somehow? Both versions (With and without comments) seem to have one\two closing parens too many.

neagle commented 9 years ago

Do you have a copy of the original file as BenKyo uploaded it? If it's different, that could definitely be useful. GoKibitz might affect the SGF somehow, though off the top of my head I would have said that the no-comments version should be the exact same SGF that was uploaded.

I did finally set up a test for WGo completely on its own with no interference from GoKibitz, and it displays the same behavior: http://wgo.nateeagle.com/wgo.js/demo6.html

neagle commented 9 years ago

Ah ha! You included it earlier, sorry!

neagle commented 9 years ago

Ugh, I'm not sure if this is bad news or good news, but it looks like Eidogo chokes on this, too. Go to move 13, then click on 2, and you'll see it place another black stone, just like WGo.

Though, just to make life more interesting, if you just load the SGF via URL, it seems to work: http://eidogo.com/#url:http://wgo.nateeagle.com/wgo.js/sgf/original.sgf

sphaso commented 9 years ago

Could it be a bug in both wGo and Eidogo? can we get Waltheri and \ or Kramer (Eidogo dev) on this? I bet they've seen their share of weird SGFs and have developed some method to debug

neagle commented 9 years ago

It could be a bug in wGo and Eidogo, though it's interesting that it would be the same bug. (I also tested it with smartgamer and it seemed like it was having the same issue.) But, notably, cGoban and SmartGo for Mac (I checked late last night), created by the father of SGF format, seem to be fine.

I just created an issue on the WGo.js project: we'll see if @waltheri has any time to help us out. (I'm not sure how active @jkk is... it's been over two years since the last activity on Eidogo. It's a bit like Mr. Kramer created the perfect jewel of a go web app and then faded into the mist. It's taken the rest of a us a while to come up with a generation of apps that do even some of the things Eidogo does any better.)

I'm sorry for the trouble: I definitely want GoKibitz to be able to host game reviews with more sophisticated variations and markup, and I'm sorry to have given BenKyo a bad first experience.

sphaso commented 9 years ago

Thanks for filing the bug report on wGo! I don't see any reason to be sorry, this SGF is proving to be a tricky one. This is a real mystery, very curious to see how it turns out :)

neagle commented 9 years ago

Well, I want GoKibitz to be give people great experiences. Since I'm certainly not making any money off this, providing actual benefit to people is pretty much the only thing I've got going. :wink:

Thanks for your collaboration on investigating this! I have to turn my attention to some other things for a while (glares at actual job), but hopefully we can resolve this mystery at some point.

neagle commented 9 years ago

This was all caused by an emoticon! :\

I actually got the tip to look specifically at character escaping issues from Anders Kierulf. I've submitted a pull request to WGo.js, and I've pushed the fix live to GoKibitz.

https://gokibitz.com/kifu/VJdTzp84?path=13

This should now be working correctly. Thanks to @xcombelle for reporting this and to @sphaso for helping work it out!

sphaso commented 9 years ago

Wow thanks! Great teamwork. I've already notified Ben. Note to future self: when testing, add a perplexed emoticon here and there :\

xcombelle commented 9 years ago

I looked at the bug fix (https://github.com/neagle/gokibitz/commit/733695263b70a2c9eb44afaa52fba1c6927b9de4) Is it this part of the file which create the bug ?

 C[So scary \:\\]

I have really hard time to understand the part of the regexp

If I understand it is done to detect the between square braquet elements shouldn't be

  \[([^\\\]]|\\.)*\]

expanded with explanation pcre with /x modifier

  \[ #an opening braquet
   ( # either
      [^\\\]] # not ( an antislash or a closing braquet )
    |  # or
    \\. # an antislash followed by any character
   )* #repeated at least 0 times
   \] # followed by a close bracket

I tested both regexp with regex101 online tool ( https://regex101.com/r/bF0jQ5/4 for compact and js one and https://regex101.com/r/bF0jQ5/2 for pcre and explained one ) It looks that both work

By the way looks like on some cases, your version eat too much for example with

 C[\\];C[a]

your version eat both comment and that between: https://regex101.com/r/bF0jQ5/5

while mine take just the first https://regex101.com/r/fB2hF2/1

neagle commented 9 years ago

I only have a moment to look at this right now, but your regex seems like an improvement in clarity, in addition to fixing the problem with a case like this C[\\];C[a] that you point out. Would you like to either comment on my pull request or create your own as an alternative?

xcombelle commented 9 years ago

I would love to make an alternative pull request, but as it would imply things like installing the software and adding tests I have still not do

neagle / gokibitz

When there is original variations in the sgf file, it is not displayed at the correct position #92