Crafty icon indicating copy to clipboard operation
Crafty copied to clipboard

Logic separation of components

Open potomak opened this issue 10 years ago • 18 comments

I think that separation of concerns in components is great and it just works, but I'd like to see only one component per file. Directories could be used to aggregate cohesive components.

See also #577.

potomak avatar Oct 25 '13 13:10 potomak

I agree completely.

dariusk avatar Oct 25 '13 14:10 dariusk

I'd like to see only one component per file.

Hmm, I'm not so certain about this as a hard rule. Some components are very short, and depend heavily on other components. For instance, should "FourWay" be in a separate file from "MultiWay"? All the former component does is call multiway. Having to constantly hop between files hurts readability just as much as having to navigate a huge monolithic file.

There are scores of components that should be in their own files, though, so I agree in general. And I think everything in its own file, while not ideal, is certainly an improvement over the current situation.

starwed avatar Oct 25 '13 19:10 starwed

I think the problem is in the files which is over 700 lines, which is the 2D.js, controls.js, core.js, math.js files.

And as you point out, it does not make any sense to split out FourWay and MultiWay, but i do not think that was what @potomak was thinking either :)

kevinsimper avatar Oct 25 '13 20:10 kevinsimper

@starwed @kevinsimper in my opinion there should be one clear rule: one file, one components.

A simple solution to the *way components problem should be to put them in the same directory (package), maybe the controls directory.

potomak avatar Oct 25 '13 22:10 potomak

A simple solution to the *way compoents problem should be to put them in the same directory (package), maybe the controls directory.

That's not a solution to the objection I had; in fact, putting the source code in subdirectories can actually make it harder to jump between files, depending on what sort of development environment you use.

starwed avatar Oct 25 '13 23:10 starwed

Note: one of the main reasons (a practical one) I'm asking for this feature is that when I'm looking for a component I'm always forced to look for the file including it using the search in project function of my editor.

potomak avatar Oct 25 '13 23:10 potomak

Putting more than one component per file is like putting more than one class per file, it's permitted in some languages. I write mostly ruby and java and it just feels wrong to me.

I challenge you @starwed to put all *way components api documentation together in one single doc page as they're in a single js file :smile:

potomak avatar Oct 25 '13 23:10 potomak

In the small amount of time I spent with Java I hated exactly that, so I think there's a large subjective component to it! :P

But clearly the current state is not great. Why don't we start by identifying files that hold multiple, unrelated components.

One very basic separation we should consider is between code that depends on a browser context, and code that doesn't. We should make it easy for someone to use Crafty code on the backend, as described in #440.

starwed avatar Oct 27 '13 18:10 starwed

If you give me the o.k. to @starwed idea to seperate browser context from rest of code, I can give it a try. If not much has changed since 0.5.3, then the seperation should be pretty simple as described in backend differences in v0.5.3

mucaho avatar Nov 02 '13 21:11 mucaho

I've gone through recently and drastically improved the organization, but there's still lots of room for improvement!

starwed avatar Jan 30 '15 19:01 starwed

@starwed I suggest we also organize the tests into the same folder/file structure as their corresponding source files.

mucaho avatar Apr 18 '15 12:04 mucaho

@mucaho Would we then split tests per the file they correspond to? That might be a good way to see where we're missing test coverage. Would require a bit of busy work, of course. :)

An intermediate step if we don't want that level of granularity yet would just be to have a test file corresponding to each folder.

starwed avatar Apr 18 '15 18:04 starwed

An intermediate step if we don't want that level of granularity yet would just be to have a test file corresponding to each folder.

That sounds like a good alternative - one test file per source folder, which is further separated into test modules corresponding to each source file. We can clearly see test coverage and avoid sparsely populated test folders.
However, that will limit our options in the long run, since not all test files inside a folder can be executed in a browser-less (node) environment. In this case it's better to have separate test files (unless there is an option to run specific test modules only).

mucaho avatar Apr 19 '15 02:04 mucaho

Additional thoughts/suggestions:

  • move collision related stuff from 2d.js into own file
  • move motion related stuff from 2d.js into own file
  • what's the deal with tests/camera.html, tests/dom_translate.html & tests/touchevents.html? are their tests not included in index.html on purpose because of compatibility reasons?
  • 2D component seems a bit complex and a lot of coupling to collision, hitboxes, graphics(dirty), etc..
    • would it be feasable to split it into smaller components (e.g. 2D_collision, 2D_graphics, ...) and communicate between them via events?
    • is relying on event callback ordering good or bad design?
    • the smaller components could be "backed" into the 2D component on crafty initialization
    • however, 2D is central component, and communication via events might introduce performance overhead

mucaho avatar May 13 '15 12:05 mucaho

  • Agreed, there's way too much stuff in 2d.js.
  • I believe some of those tests needed special setup or features that didn't exist in phantomjs? It's been a while since I've looked at them.
  • I suppose we could split the parts of "2D" that are related directly to graphics out: alpha levels, flipping the display, etc. I don't think there would be much (if any) perf impact from doing this. I'm not sure I see much point in splitting it further -- you'd have things that could apply to any 2D object, and then things that only matter if it's being rendered.

starwed avatar May 14 '15 20:05 starwed

I'm not sure I see much point in splitting it further -- you'd have things that could apply to any 2D object, and then things that only matter if it's being rendered.

Sounds good. My primary concern is about maintainability - it's a bit difficult to see everything that gets triggered when you modify an entity's coordinate. That's why splitting it even further (the collision stuff, the propagation to children, etc...) could maybe also make sense.

mucaho avatar May 14 '15 22:05 mucaho

@starwed Also, what do you think about splitting the "DOM" layer and components into "DOM"(2D transforms) and "DOM3D" (3D transforms) - then it would be similar to "Canvas" and "WebGL". If you're worried about browser compatibility you would use "DOM" and "Canvas". If you want state-of-the art drawing you would use "DOM3" and "WebGL".

mucaho avatar Jul 17 '15 17:07 mucaho

Recent mouse suggestions got me thinking:

  • Make viewport (the actual camera transformations) a system
  • Split/move the stage stuff (the actual DOM entity, its dimensions and initialization) out of viewport and make it also a system
    • this ensures stage is only initialized once (#956)
    • you can listen & trigger mouse events on this stage - it serves kind of like a background entity

Also, the events for or triggered by these (hopefully soon-to-be systems) would then be localized to each system, rather than triggered globally on the Crafty object.

mucaho avatar Mar 07 '16 13:03 mucaho