flame-engine / flame

A Flutter based game engine.
https://flame-engine.org
MIT License
8.98k stars 879 forks source link

feat: Expose vector classes on core file [flame_3d] #3211

Open luanpotter opened 3 days ago

luanpotter commented 3 days ago

Description

Expose vector classes on extensions file.

It is very common that we want to work with vector and matrices even though I don't want to import anything else specific from Flame. This PR allows we to use the extensions file as a base import similar to how it is on normal flame.

Checklist

Breaking Change?

luanpotter commented 3 days ago

On regular Flame we have extensions for all the "base" vector classes, so I typically import extensions as the baseline file when I don't want to have game, components, etc. This mimics that behaviour even though in Flame 3d we don't have extensions for all vector classes yet.

If we don't expose them, I cannot import as vector_math is an indirect dependency.

They are similarly exposed on the flame_3d.dart file and others, but I don't want to bring in all the other dependencies that comes with that.

I feel the extensions file is like an "utils" file that I can import everywhere, even though I am not working with flame stuff.

spydon commented 3 days ago

I feel the extensions file is like an "utils" file that I can import everywhere, even though I am not working with flame stuff.

Except that it is not, it is a barrel file for extensions, very nicely contained. :smile:

luanpotter commented 3 days ago

It is that given its contents, but from the point of view of the user, it is an utilities file. It is the file from which I import to get all the base classes likes vector, color, etc. enhanced with Flame helpers. The way I see it, when I import extensions.dart, I want to use flame's Vector2, which is the base Vector2 + some extensions under the hood.

Otherwise I have no way of accessing the base classes like Vector3, Matrix4 that I need, other than importing the whole flame_3d stuff, which is out of scope for a lot of the internal code I am writing.

spydon commented 3 days ago

Ooh this is on flame_3d, I thought this was the general one. We are doing similar dirty things here: https://github.com/flame-engine/flame/blob/main/packages/flame/lib/src/extensions/vector2.dart#L6

Why are you worried about importing the full flame_3d barrel file? Are there possible collisions, or due to that you can get more suggestions in the IDE? Just that it has more things in it doesn't really matter in the end due to the tree shaking (except for possible dev experience concerns of course).

but from the point of view of the user, it is an utilities file

I don't think it should be though...

luanpotter commented 3 days ago

We are doing similar dirty things here

Yeah, this matches that behaviour. I wouldn't say it is dirty at all in this case, maybe should just be explicit on the types to import (vectorX, matrixX). but I think it should export those.

the broad dart:ui I agree is totally bad and uncesseary

Why are you worried about importing the full flame_3d barrel file?

Are there possible collisions: yes you can get more suggestions in the IDE: no it have more things in it doesn't really matter: yes due to the tree shaking: no

if we are going to have everyone import a file that exports everything we don't even need the separate files at all right? there would be only one file for the entire repo.

there is code that cares nothing about flame_3d stuff, like the gltf parser I am writing, yet I need vector and matrix. it's a matter of logical separation of concerns grouped into nice imports

I don't think it should be though...

I completely disagree, I feel it makes sense to have a base file to export the fundamental constructs. I often see a file within our repo that imports for example components.dart but it has nothing to do with components at all, it just wants vector2. I always replace those with extensions.dart. maybe not the best name for the file, I agree, but I think it serves a good purpose. these export files should be related to the areas that the user want to export, not the exact nature of its contents. the user wants to use Vector2, he shouldn't care how the sausage is made.

spydon commented 3 days ago

the user wants to use Vector2, he shouldn't care how the sausage is made.

Just exporting each class that has an extension in it seems fine for sure! But exporting 23 external classes from an extensions barrel doesn't seem intuitive at all, I would never guess that I all of a sudden could get name clashes on pure functions like these just because I imported an extensions barrel file.

completely disagree, I feel it makes sense to have a base file to export the fundamental constructs

Sure, but we can name it something better! Instead of re-using something that already is defined by its name what it should do.

spydon commented 3 days ago

Another thing, flame_3d should probably explicitly have vector_math as a dependency since it is tightly dependent on it.

luanpotter commented 3 days ago

Just exporting each class that has an extension in it seems fine for sure! But exporting 23 external classes from an extensions barrel doesn't seem intuitive at all

I think it's neither here nor there - it should export the base classes to work with flame regardless if we happen to have an extension or not. so basically VectorX and MatrixX in flame_3d case. def not the whole 23 classes, I agree

Sure, but we can name it something better

I don't mind calling it extensions.dart but I don't have anything against renaming it at all, open to any suggestion

flame_3d should probably explicitly have vector_math as a dependency

It gets it through flame already. do you mean add it just as a means of "documenting" it more explicitly? if so I am not opposed at all

spydon commented 3 days ago

I don't mind calling it extensions.dart but I don't have anything against renaming it at all, open to any suggestion

Not renaming it, but keeping this and then have a file called something else that contains base classes needed to work with the library (and the extensions can be exported from there).

It gets it through flame already. do you mean add it just as a means of "documenting" it more explicitly? if so I am not opposed at all

Yes, we shouldn't rely on Flame always exporting the full vector_math library. You even get this by the analyzer now:

  /home/runner/work/flame/flame/packages/flame_3d/lib/extensions.dart:1:8 The imported package 'vector_math' isn't a dependency of the importing package.
  Notice: Try adding a dependency for 'vector_math' in the 'pubspec.yaml' file.
  See https://dart.dev/lints/depend_on_referenced_packages to learn more about this problem.