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

next: Support Seamless Texture & Enhance Stone Quality #366

Merged
merged 6 commits into from
Oct 1, 2018

Conversation

zsalch
Copy link
Contributor

@zsalch zsalch commented Sep 18, 2018

#361

  1. The change support seamless texture for background and board.
  2. Enhance the stone draw quality

image

Image stone = theme.getWhiteStone(new int[]{x, y});
g.drawImage(stone, centerX - stoneRadius, centerY - stoneRadius, stoneRadius * 2 + 1, stoneRadius * 2 + 1, null);
// Enhance draw quality
BufferedImage stone = theme.getWhiteStone(new int[]{x, y});
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is copy pasted 4 times in this PR... Do you think you could refactor that method to avoid the duplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, of course. I have changed and committed. Thanks.

@ParmuzinAlexander ParmuzinAlexander mentioned this pull request Sep 18, 2018
Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain left a comment

Choose a reason for hiding this comment

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

LGTM!

@zsalch
Copy link
Contributor Author

zsalch commented Sep 20, 2018

@OlivierBlanvillain
Sorry for I updated the code for improve the stone display performance.
Please take time to review, thank you.

/**
* Draw scale smooth image, enhanced display quality
*/
public void drawScaleSmoothImage(Graphics2D g, BufferedImage img, int x, int y, int width, int height, ImageObserver observer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Dead code?

/**
* Draw scale smooth image, enhanced display quality, less and faster than drawScaleSmoothImage
*/
public void drawScaleImage(Graphics2D g, BufferedImage img, int x, int y, int width, int height, ImageObserver observer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also dead code??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the both functions are dead code, but the drawScaleSmoothImage can provide high quality scale draw, the drawScaleImage can provide better quality than the default and faster than the drawScaleSmoothImage, so I think it might be useful to keep it.
Of course, if it doesn't allow exists, I will remove them.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think it might be useful to commit these functions even if they are unused, then please put the code inside a comment with an explanation of what it does differently that the one that's actually used...

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 have described & disabled the both functions.

g.drawImage(stone, centerX - stoneRadius, centerY - stoneRadius, stoneRadius * 2 + 1, stoneRadius * 2 + 1, null);
// Enhance draw quality
int size = stoneRadius * 2 + 1;
g.drawImage(getScaleStone(stone, color, size, size), centerX - stoneRadius, centerY - stoneRadius, size, size, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there is also a lot of copy pasting here... For fancy-stones, I think the entire switch statement can be replaced by 6 lines:

if (uiConfig.getBoolean("fancy-stones")) {
    boolean isBlack = color == BLACK || color == BLACK_GHOST || color == BLACK_CAPTURED;
    boolean isGhost = color == BLACK_GHOST || color == WHITE_GHOST;
    drawShadow(gShadow, centerX, centerY, isGhost);
    Image stone = isBlack ? theme.getBlackStone(new int[]{x, y}) : theme.getWhiteStone(new int[]{x, y});
    int size = stoneRadius * 2 + 1;
    g.drawImage(getScaleStone(stone, color, size, size), centerX - stoneRadius, centerY - stoneRadius, size, size, null);
} else {
  // code for non-fancy stones
}

By the way, do you know why do we keep two code paths here? Could we keep only fancy-stones and remove the other one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you're right.
Because there are 'fancy-stone' settings in the configuration file, some people may like simple styles.

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 have simplified code.

case BLACK_CAPTURED:
case BLACK_GHOST:
if (cachedBlackStoneImage == null || cachedBlackStoneImage.getWidth() != width || cachedBlackStoneImage.getHeight() != height) {
stone = new BufferedImage(width, height, BufferedImage.TYPE_INT_ARGB);
Copy link
Contributor

Choose a reason for hiding this comment

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

This entire block is copy pasted, could you please factor it out?

@OlivierBlanvillain
Copy link
Contributor

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants