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

updated normal polygon and circle colour #79

Closed
wants to merge 4 commits into from
Closed

updated normal polygon and circle colour #79

wants to merge 4 commits into from

Conversation

enjiruuuu
Copy link

Closes #78

@enjiruuuu
Copy link
Author

i just realised this issue doesn't have the help wanted tag, can I still make this PR?

@getkey
Copy link
Owner

getkey commented May 27, 2022

Of course!
But please use the same formula that's used in game, which I digged up below:

import { lab } from 'd3-color';
import { interpolateLab } from 'd3-interpolate';
import { utils } from 'pixi.js';

export const darkSlateGray = 0x121f1f;

lab(
	interpolateLab(
		utils.hex2string(color),
		utils.hex2string(darkSlateGray),
	)(0.7),
).formatHex()

Can you do it for all the blocks?

@enjiruuuu
Copy link
Author

should i be using the exact same formula? from what i've seen, the color will turn out to be way darker than expected. for example, this:

Screenshot 2022-05-29 at 4 01 44 PM

versus this:

Screenshot 2022-05-29 at 4 04 01 PM

Otherwise, do you mean to use a similar method like this?

utils.string2hex(
	lab(
		utils.hex2string(color)
	).formatHex(),
)

@getkey
Copy link
Owner

getkey commented May 29, 2022

Oh yeah, that's because in game the inside of the block is very dark, and the line around isn't.

image

Here is the formula for the line.

export function getLineColor(color) {
	return utils.string2hex(
		lab(utils.hex2string(color))
			.brighter(1.2)
			.formatHex(),
	);
}

You can try adding the line while keeping the dark fill.

If adding the line is too complicated, just play around with the interpolateLab parameter until you get something that looks decent, maybe with lower values like 0.1 or 0.2 instead of 0.7.

@enjiruuuu
Copy link
Author

gotcha, thanks! I've made the updates to the PR.

Given that the same formula (originally getStrokeColor) is used now across multiple places, I thought that getFillColor is a more accurate name for the method.

Copy link
Owner

@getkey getkey left a comment

Choose a reason for hiding this comment

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

image

Whoa looks very good! 😄

I found a bug: if you click the bouncy block, the selection box doesn't appear.
selection_bug_test_case.json.txt (rename this file to JSON)

I tried playing around with src/components/Polygon.ts and src/components/SelectionAabb.ts to understand what's going on, but I found nothing particular 😕. It might be a bug in Pixi, so you can try making a minimal reproduction in pixiplayground (make sure to use the same version of Pixi).

src/components/PixiApp.tsx Outdated Show resolved Hide resolved
src/components/Circle.ts Outdated Show resolved Hide resolved
src/components/Polygon.ts Outdated Show resolved Hide resolved
@enjiruuuu
Copy link
Author

thank you! 😄 I've updated just the syntax for now and will continue investigating the bug.

From what I've seen, it seems to be happening when I add the instance.lineStyle method in Polygon.ts . It's strange because it seems to only be happening for the second polygon added, regardless of type.

@enjiruuuu
Copy link
Author

@getkey what are the steps you took to trigger the bug?

I have tried clearing my level and adding 2 polygons, then exporting them and importing them back in but that works ok for me. I've only been able to replicate the issue by loading in the file you have provided but was wondering what steps were taken to create that particular file.

@getkey
Copy link
Owner

getkey commented Jun 18, 2022

This is a Pixi.js bug, which I've reported upstream: pixijs/pixijs#8409. Let's wait until it is resolved in Pixi.js.

@getkey
Copy link
Owner

getkey commented Aug 10, 2022

@enjiruuuu I upgraded to the latest Pixi.js, and surely enough the selection bug is gone. 🙂

One remaining problem: the paints get block styles. Example:
image

I'd make a new BlockPolygon component that would be used for all blocks, and keep the old Polygon to be used for paints. (In the future there could be even more specialized Polygons like IcePolygon with the ice texture, but I think this is good enough for now 🙂 )

@enjiruuuu
Copy link
Author

@getkey sweet, thanks for the update!

I've made the fix in the recent commit, do let me know if this is what you meant haha incase I misunderstood something 😬

@getkey
Copy link
Owner

getkey commented Aug 13, 2022

Mhh I'm having second thoughts, because I realize now that BlockPolygon and Polygon are 90% copy-paste of each other.

You can make Polygon more generic. Make it purely about drawing, and move the logic of what to draw higher in the hierarchy. Let it accept:

  • the vertices
  • the fill color
  • the stroke color
  • the stroke width (can be 0)
  • the texture (can be null)
  • etc

Then you can make 2 React components: Paint.tsx and Block.tsx. They both rely on ModifiablePolygonand know what are the right styles for a paint or a block. I prefer that compared to adding addType to ModifiablePolygon, because in my experience you don't want to cram conditional config in a single component, that will turn in technical debt.

Note: ModifiablePolygon magically forwards the props to Polygon, but let me know if the type-checker gives you trouble.

@enjiruuuu enjiruuuu closed this by deleting the head repository Mar 18, 2023
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.

Use the same colors for the block as in game
2 participants