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

Add custom face color calculation function #3

Merged
merged 5 commits into from
Mar 15, 2024

Conversation

InstrinsicAutomations
Copy link
Contributor

@InstrinsicAutomations InstrinsicAutomations commented Mar 14, 2024

  • Optional custom face color calculation callback function.
  • Fall-back to default (original) face color calculation function.
  • Face class to hold face data and calculate a unit normal (cached).
  • Made use of the vector math library functions and removed the hand-made functions.
  • Moved the asserts in the previous PR to be next to the ones in the original code (in the constructor).
  • Left an example how to use the face color calculation callback function.
  • Provided 2 new screenshots: Fresnel (example.dart) and glass diffraction approximation.
  • Updated readme.

New:

  • Added new linter rules and applied all necessary changes to comply with linter.
  • Replaced vector_math with vector_math_64 to avoid unnecessary casting. Similar issue.
    • Dart will turn those offsets into Float32 buffers at the end anyway for us.

Comment on lines 344 to 346
face = faceColorFunc == null
? _defaultFaceColor(face)
: faceColorFunc!(face);
Copy link
Owner

Choose a reason for hiding this comment

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

Could also do something like this: face = faceColorFunc?.call(face) ?? _defaultFaceColor(face);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't know about this! Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 341 to 342
Face face = Face(verticesToDraw[faceIdx[0] - 1],
verticesToDraw[faceIdx[1] - 1], verticesToDraw[faceIdx[2] - 1]);
Copy link
Owner

Choose a reason for hiding this comment

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

I think it would be a bit nicer to extract these function parameters above this call so its a bit more readable

Suggested change
Face face = Face(verticesToDraw[faceIdx[0] - 1],
verticesToDraw[faceIdx[1] - 1], verticesToDraw[faceIdx[2] - 1]);
final v1 = verticesToDraw[faceIdx[0] - 1];
final v2 = verticesToDraw[faceIdx[1] - 1];
final v3 = verticesToDraw[faceIdx[2] - 1];
Face face = Face(v1, v2, v3);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@arnovanliere
Copy link
Owner

I've also added a workflow to lint and analyze the code base. You should probably first merge that into your own branch so the workflow can run on this pull request as well. This is also necessary to resolve the merge conflicts.

@InstrinsicAutomations
Copy link
Contributor Author

The only linter not addressed was the request to change explicit types to var. Correct me if I'm wrong but this is an out-dated variable type with Dart is it not? I'm ok with final variable_name = expression because at least the expression resolves to a type. But when there's no right-hand expression, being forced to use var is generally going to be unreadable.

@arnovanliere
Copy link
Owner

The two list initializations can be written like this

final List<Offset> offsets = [];
final List<Color> colors = [];

If you use a constructor, it is redundant to also provide a type for the variable, so

Face face = Face(...)

can be written like

var face = Face(...)

But as I was writing this, I read the style guide for the Flutter repo and this recommends to always explicitly type variables (as you were doing). If you can find the right combination of rules to enforce this style, go ahead and change them. I do honestly not really care for which code style is used, as long as it is consistent and a common set up in other Flutter projects

@arnovanliere arnovanliere merged commit 1a98574 into arnovanliere:master Mar 15, 2024
3 checks passed
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.

2 participants