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

C++ Engine refactor #116

Merged
merged 26 commits into from
Jun 5, 2020
Merged

C++ Engine refactor #116

merged 26 commits into from
Jun 5, 2020

Conversation

mindhells
Copy link
Member

No (intended) behavior change in this PR, perhaps some minor improvements (loops, temp vars, repeated/unneeded operations).

  • New entity "calcoptions" to avoid carrying all its properties as constructor parameters for underneath classes.
  • Introduced the use of unique_ptr as well as reviewed some class initializations to simplify.
  • remove some unused/unreachable code.
  • rename and reorganize members for some classes to clarify public interfaces and the use of private members.
  • introduced some std utilities like vector.
  • started working towards SOLID principles (specially "single responsibility")
  • update old fashion idioms like c-style type casting
  • ...

* for this PR I want to keep the current fract4dc interface. This impedes some decoupling (at least without introducing more complexity on the python extension implementation). For example: STFractWorker is a friend class of fractFunc. That's an already identified code smell, and you can can see how python tests using fw_create need to, without apparent reason, to create a fractFunc when they only want to test the worker.

@mindhells mindhells changed the title Engine refactor C++ Engine refactor May 28, 2020
};

inline bool operator== (const s_rgba& a, const s_rgba& b) {
// according to the original implementation "a" is not considered in the comparison
Copy link
Member

Choose a reason for hiding this comment

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

I can't honestly remember why that would be 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's because the alpha it's not saved in the image buffer:

rgba_t image::get(int x, int y) const
{
    char *start = buffer + x * 3 + y * m_Xres * 3;
    //assert(start  + 2 - buffer <= bytes());
    rgba_t pixel;
    pixel.r = start[RED];
    pixel.g = start[GREEN];
    pixel.b = start[BLUE];
    pixel.a = 0;
    return pixel;
}

@edyoung
Copy link
Member

edyoung commented May 30, 2020

looks good overall, thanks. Calcoptions seems a nice improvement. I've never personally found the m_ convention for member variables all that useful but I don't mind having it.

@mindhells mindhells marked this pull request as ready for review June 1, 2020 12:17
@mindhells
Copy link
Member Author

I've rebased this branch with master. I plan to do further refactor but since you've already reviewed most of it... feel free to merge.
About the m_convention: I think it's ugly but helpful when looking at large member functions. That being said you can rely on the IDE for that kind of help or maybe a better-looking convention/style. If you have any particular preferences there I'd be glad to follow them

Right now I'm trying to get rid of the "friend" coupling between fractFunc and STFractWorker. I'm wondering about the best strategy for them to share the information they're sharing now and, if possible, reduce it.
Seems fractFunc is holding a lot of responsibilities:

  • calculating 4D geometry
  • reporting calculation progress and events through a site
  • high level stats-based predictions (auto-deepen and auto-tolerance): this updates the calcoptions
  • manage workers (request for work to be done, flush pending work, get stats from them)
    On the other hand the STFractWorker depends on the geometry, the calcoptions (that might be updated by fractFunc), ...
    One of the benefits I'd like to provide with this is to ease tasks like introducing arbitrary precision, or any other complex enhancements on the roadmap.

@mindhells
Copy link
Member Author

mindhells commented Jun 5, 2020

Hey guys, for what it's worth, I'm finished with this PR. I didn't do much since your review except for rebasing with master.
On the other hand I'm working on decoupling fractFunct and STFractWorker (friend classes for the time being). If you are curious how it's going check 8cd17f6 (feel free to leave any comments, I'd appreciate them). PR will come after this PR, since it's based on its status.

@edyoung
Copy link
Member

edyoung commented Jun 5, 2020

Thanks :-)

@edyoung edyoung merged commit 73d0aa4 into fract4d:master Jun 5, 2020
edyoung added a commit that referenced this pull request Jun 7, 2020
* work in progress: create markdown-based manual

* finished first pass through manual.md

* exclude parsetab and lextab from pylint (#102)

* Remove buildtools package (#104)

No executable files ending in get.py are being installed.

* Add pylint singleton-comparison check (#105)

* Remove unused traceback import (#106)

* fix #107: wait until fd has something to read or fail (#108)

* Arbitrary precision formula experiment (#103)

* examples: allow custom formula filename in cmake

* [#7] example experiment: formula with arbitrary precision support

* MP example: add swap idiom and other improvements

* add debug information to cpp examples compilation

* release prep for 4.2 (#111)

* Update http links in README.md and setup.py to https (#115)

* Starting point for a benchmark to compare multiple-precision libs (#112)

* typo

* experiment to benchmark arbitrary precision math

* was running 3 benchmarks

* update #112 (#113)

* install google benchmark globally in exmaples infrastructure

* update exmaples readme

* disable frequency scaling during benchmark

* remove i/o from benchmark loop

* benchmark several different bit lengths

* Basis for discussion on available math libraries

* notes

Co-authored-by: Alberto Gonzalez <[email protected]>

* tweak doc generation process

* Check gnofract4d executable with pylint (#117)

* Install icons into the hicolor theme (#121)

gnofract4d-logo.png is size 640x640.

Individual icons created using ImageMagick, e.g.:
magick gnofract4d-logo.png -strip -resize 256x256 logo/256x256/gnofract4d.png

Install the 48x48 icon into pixmaps.

* Run pylint as a separate job (#118)

Report linting and testing results separately. Only run pylint once.

* C++ Engine refactor (#116)

* tidy worker initialization

* remove unused typedef

* worker: improve AA comparison

* fix docker test script

* remove duplicate extern declaration

* workers: refactor common members into base

* remove some completed todo's

* organize fractfunc initialization and members

* unravel coupling: pointfunc - site

* refactor: extract calculation options into a struct to ease initializers

* remove duplicate

* move rgba type basic operations to its type definition

* remove some temporaries in stworker->work

* review worker member names

* remove unneeded worker alloc members

* remove pointFunc factory

* update some old-fashion idioms on fractfunc and stats

* prefix fractfunc private members

* avoid clearing fates twice when autoupdating iters and period tolerance

* reorganize and narrow down public/private worker interface

* remove unused, mark experimental and review old fashioned code on stfractworker

* calcoptions: add some comments and move asynchronous back to where it belongs

* prevent uninitialized members on fractFunc

* remove initialization success flag on workers

* fix comment

* reorganize fractfunct members and remove unused

* work in progress: create markdown-based manual

* finished first pass through manual.md

* tweak doc generation process

* generate manual with hugo

new directory 'manual' generates standalone HTML for manual

* rest of hugo-based manual

* repoint to our copy of theme

* repoint to our version of theme

* install hugo-generated manual

* update submodule ref

* ignorance

* delete docbook version of manual. so long!

* fix test for doc version

* fix submodule commit

* install hugo in travis

* better errors on doc generation

* more debug output

* jfc

* apt version of hugo is too old, try this

* maybe this

* fold createdocs into setup

* delete old files

* work in progress: create markdown-based manual

* finished first pass through manual.md

* tweak doc generation process

* work in progress: create markdown-based manual

* generate manual with hugo

new directory 'manual' generates standalone HTML for manual

* rest of hugo-based manual

* repoint to our copy of theme

* repoint to our version of theme

* install hugo-generated manual

* update submodule ref

* ignorance

* delete docbook version of manual. so long!

* fix test for doc version

* fix submodule commit

* install hugo in travis

* better errors on doc generation

* more debug output

* jfc

* apt version of hugo is too old, try this

* maybe this

* fold createdocs into setup

* delete old files

* delete gui createdocs

It's more trouble than it is worth.
Getting access violations when calling it from setup.py.
Since this doesn't change much anyway,
easier to maintain commands.html manually

* setup.py updates

distutils to setuptools
make doc generation a custom build step

* merge madness

* build manual first

* custom build command doesn't work :-(

* back to generating docs separately

* checkin generated doc files for consistency

* restore css file

* move css file inside fract4dgui

* fix finding resources after install

* guess which pylint to use

* typo

* disable that darn test

* fix pylint whining

Co-authored-by: Alberto Gonzalez <[email protected]>
Co-authored-by: Chris Mayo <[email protected]>
@mindhells mindhells deleted the engine-refactor branch June 8, 2020 08:19
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