inital sgf download

[SameerDalal]

Currently the SGF data for a quantum game is stored in the this.sgfContent variable in quantum.ts. For the user to download this, I also created a download button in DownloadSGF.vue, in the GameView folder, however I'm not sure how to move the contents of the this.sgfContent string from the typescript file to the vue file.

[benjaminpjones]

This is a nice start!

I think the main part that is missing is the server endpoint. That will probably go in server/src/api.ts. games/:gameId is the most similar endpoint to what we'll need for SGF, except instead of a JSON response, we'll send a text file.

router.get('/games/:gameId/sgf', function(request, response){
  try {
    // get the moves and other game info from database
    const game: GameResponse = await getGame(req.params.gameId);
    // convert to sgf format
    const sgfString: string = toSGFString(game.moves, game.players);

    // pipes a file from memory to the client
    // mostly copied from
    // https://stackoverflow.com/questions/45922074/node-express-js-download-file-from-memory-filename-must-be-a-string
    var fileContents = Buffer.from(sgfString, "base64");
    var fileName = `${req.params.gameId}.sgf`
  
    var readStream = new stream.PassThrough();
    readStream.end(fileContents);

    response.set('Content-disposition', 'attachment; filename=' + fileName);
    response.set('Content-Type', 'text/plain');

    readStream.pipe(response);
  } catch (e) {
    // Express 4 doesn't handle async errors very well unfortunately, so we have to send the error manually
    res.status(500);
    res.json(e.message);
  }
});

Disclaimer: this is just me copying code around - I haven't tested myself. Let me know if you run into any issues with this!

---

I haven't referenced QuantumGo directly. This is because I noticed your current implementation could work reasonably well for other variants (except maybe the SGF header), so it would be a lot simpler to have the SGF implementation decoupled from the variant itself. If you foresee the implementation diverging from standard SGF/Go, we can figure out a way to let variants define their own SGF function, just let me know.

[benjaminpjones]

Instead of a downloadData callback, we can probably make this an <a href="/games/${gameId}/sgf">Download Game Files</a>

(Content-Disposition: attachment will instruct the browser to save the file)

[benjaminpjones]

This probably belongs on the GameView itself, not the NavButtons

[SameerDalal]

Fixed!

[benjaminpjones]

I haven't referenced QuantumGo directly. This is because I noticed your current implementation could work reasonably well for other variants (except maybe the SGF header), so it would be a lot simpler to have the SGF implementation decoupled from the variant itself. If you foresee the implementation diverging from standard SGF/Go, we can figure out a way to let variants define their own SGF function, just let me know.

On second thought, we probably do need a per-variant implementation, esp. since some variants don't just have black and white. So the way we would do that I think is to expose the SGF via a new optional member of AbstractGame:

class AbstractGame {
    ...
  sgfContent?: string;
}

Then use that in the express endpoint:

  // get the moves and other game info from database
  const game: GameResponse = await getGame(req.params.gameId);
  // When game.variant == "quantum", this creates an instance of QuantumGo
  const game_obj: AbstractGame = makeGameObject(game.variant, game.config);
  // play out the game
  game.moves.forEach((moves) => {
    const { player, move } = getOnlyMove(moves);
    game_obj.playMove(player, move);
  });
  // For backwards compatibility, variants don't have to define sgfContent
  if (game_obj.sgfContent === undefined) {
    throw new Error(`SGF not supported for variant ${game.variant}`)
  }
  const sgfString: string = game_obj.sgfContent;

[benjaminpjones]

There is a TS error in CI:

error TS2345: Argument of type '{}' is not assignable to parameter of type 'NonNullable<(Partial<{ readonly [x: number]: string; }> & Omit<(readonly string[] | { readonly [x: string]: Prop<unknown, unknown> | null | undefined; }) & (VNodeProps & AllowedComponentProps & (ComponentCustomProps & Readonly<...>)), never> & Record<...>) | (Partial<...> & ... 1 more ... & Record<...>)> & Record<...>'.

It's not entirely obvious from the error message, but it will be resolved once you add some non-empty props here. (you could also fix the error by removing defineProps(), but I think we'll want to pass in gameId at least)

[SameerDalal]

Fixed!

[merowin]

Nice start, thank you for working on this!

Recently we made some changes that enable graph boards for certain variants, including quantum, see e.g. #264
For grid boards, the moves are stored represented as sgf coordinates, but not for non-rectangular boards.

To distinguish between the two cases, there is the function isGridBadukConfig

[SameerDalal]

I haven't referenced QuantumGo directly. This is because I noticed your current implementation could work reasonably well for other variants (except maybe the SGF header), so it would be a lot simpler to have the SGF implementation decoupled from the variant itself. If you foresee the implementation diverging from standard SGF/Go, we can figure out a way to let variants define their own SGF function, just let me know.

On second thought, we probably do need a per-variant implementation, esp. since some variants don't just have black and white. So the way we would do that I think is to expose the SGF via a new optional member of AbstractGame:

class AbstractGame {
    ...
  sgfContent?: string;
}

Then use that in the express endpoint:

  // get the moves and other game info from database
  const game: GameResponse = await getGame(req.params.gameId);
  // When game.variant == "quantum", this creates an instance of QuantumGo
  const game_obj: AbstractGame = makeGameObject(game.variant, game.config);
  // play out the game
  game.moves.forEach((moves) => {
    const { player, move } = getOnlyMove(moves);
    game_obj.playMove(player, move);
  });
  // For backwards compatibility, variants don't have to define sgfContent
  if (game_obj.sgfContent === undefined) {
    throw new Error(`SGF not supported for variant ${game.variant}`)
  }
  const sgfString: string = game_obj.sgfContent;

I used this implementation and created the proper API endpoints in api.ts and index.ts. Whenever the download button is clicked the link is changes to /game/{gameID}/sgf, which is correct. However in api.ts, the router.get function, which executes the download, is not being called. I assumed that the <a> tag would automatically create a GET request to the router, so I'm not sure why the code below is not being called.

api.ts

router.get('/game/:gameId/sgf', async (req, res) => {
  //route not being called
  console.log("test");
  res.status(200).send("test");
  try {

    const game = await getGame(req.params.gameId);

    const game_obj = makeGameObject(game.variant, game.config);

    game.moves.forEach((moves) => {
      const { player, move } = getOnlyMove(moves);
      game_obj.playMove(player, move);
    });

    if (game_obj.getSGF() === '') {
      throw new Error(`SGF not supported for variant ${game.variant}`);
    }
    
    // download sgf
    res.download(game_obj.getSGF());
  } catch (e) {
    res.status(500).json({ error: e.message });
  }
});

DownloadSGF.vue

<script setup lang="ts">


const props = defineProps<{ gameId?: string}>();

</script>

<template>
  <a :href="`/game/${props.gameId}/sgf`">Download Game Files</a>
</template>
  
  <style scoped>
  button {
    margin-top: 10px;
  }
  </style>

[benjaminpjones]

Whenever the download button is clicked the link is changes to /game/{gameID}/sgf, which is correct. However in api.ts, the router.get function, which executes the download, is not being called.

Ah, I forgot - we prepend /api to all the routes in api.ts. If you do /api/game/{gameID}/sgf, the route will be called. One way to check server endpoints without client is to visit localhost:3001 directly (in this case localhost:3001/api/game/:id/sgf) - that's how I knew it was a problem with the URL and not the <a> tag.

---

There is another issue once you get that fixed. res.download() takes a filepath, not the file content, which is why I recommend trying the pipe stuff from this SO thread:

Edit: I had some "stream" stuff here before, but I found it can be much simpler:

    // download sgf
    res.set({
      "Content-Disposition": `attachment; filename="${req.params.gameId}.sgf"`,
    });
    res.set("Content-Type", "text/plain");
    res.send(game_obj.getSGF());

[SameerDalal]

Thank you for your help! The download SGF feature should be complete and is ready to be merged into the main branch.

[benjaminpjones]

Ah I think I understand your comment on the other PR now

Are there any benefits to creating a more complex test?

I had written a standalone test, which has some benefits, but this is fine too :)

[SameerDalal]

Perfect!

[benjaminpjones]

For my own understanding is there something about SGF that requires the game to be finished? This is totally fine regardless, just wondering if we should consider implementing mid-game sgf download in the future.

[SameerDalal]

We could definitely implement a mid-game sgf download if needed; it wouldn't require that much modification.

[SameerDalal]

We could definitely implement a mid-game sgf download if needed; it wouldn't require that much modification.

We could definitely implement a mid-game sgf download if needed; it wouldn't require that much modification.

Now that I think about that, our current implementation relies on the game to be over before it saves the entire sgf. It is still possible but would require a different approach

[benjaminpjones]

Nice work, thank you Sameer! I'll test a bit then merge later tonight or tomorrow

[SameerDalal]

Nice work, thank you Sameer! I'll test a bit then merge later tonight or tomorrow

No problem! I'll do one more commit with the updated yarn lock!

[SameerDalal]

Nice work, thank you Sameer! I'll test a bit then merge later tonight or tomorrow

No problem! I'll do one more commit with the updated yarn lock!

Nice work, thank you Sameer! I'll test a bit then merge later tonight or tomorrow

No problem! I'll do one more commit with the updated yarn lock!

The yarn lock is good as is!

[benjaminpjones]

When merging, I noticed lints and tests failed. Here are the commands to run them locally:

yarn lint
yarn test

I have a precommit hook set up to run these before every commit, so I don't have to think about it. Pretty easy to set up - just add a file named .git/hooks/pre-commit with the following:

yarn lint:check
yarn test

[benjaminpjones]

Changes are live!