nkh / P5-App-Asciio

Plain ASCII diagram
https://nkh.github.io/P5-App-Asciio/
56 stars 4 forks source link

Refactor Cross.pm #52

Closed nkh closed 1 year ago

nkh commented 1 year ago

@qindapao

I had a look at cross .pm, I'll look at Ascii.pm when I have time, and I refactored it a bit in this branch https://github.com/nkh/P5-App-Asciio/commits/refactor_cross

I propose you take that code and continue refactoring it. I haven't tested it but I feel confident that

The things I recommend:

Once the code is refactored we can discuss how to make it faster if necessary, I have some ideas. What's the size it starts to feel slow? tens of boxes, hundreds, thousands?

qindapao commented 1 year ago

@nkh Your suggestions are very good, I will continue to refactor according to your suggestions, it just needs to take some time.

Regarding performance, I will test it as soon as possible.

Performance bottlenecks mainly come from three aspects:

  1. transform_elements_to_ascii_two_dimensional_array_for_cross_mode The loop nesting of this function is relatively deep, and I don't know the specific performance.And characters are processed one by one.
  2. In fact, we don't care about the text in the box, but it also participates in the calculation. However, they cannot be ignored because of the problem of character coverage.One problem with ignoring them is that hidden cross elements are considered to be in the foreground, generating fillers that shouldn't be generated.
  3. for $cross_index (@{$index_ref}) It may be that when there are too many points filled, the cycle base will become larger.

Leave all of this for later, let me solve the basic refactoring problem first. In fact, in order to optimize performance, I have used two caches.

nkh commented 1 year ago

You just can merge my changes and check them (I did some test runs but not enough)., I won't touch that file again. performance later, I agree,

I tried with a just ten or so boxes and arrow and it was fast enough then.

since you'll be refactoring continuously I'll close this ticket.