Ticket #1818 (closed unplanned_task: obsolete)

Opened 10 years ago

Last modified 9 years ago

GROUP_SCENES_R1

Reported by: gogov Owned by: gogov
Priority: major Milestone: M10_BETA2
Component: BASE_SCENE Version: 2.0
Keywords: scenes, sprites, tiles, java3d, gl, jogl, native, deployment Cc: deni, gogov, izomorphius
Category: BASE Effort:
Importance: Ticket_group:
Estimated Number of Hours: 0.0 Add Hours to Ticket: 0
Billable?: yes Total Hours: 0
Analysis_owners: deni, gogov, izomorphius Design_owners: deni, gogov, izomorphius
Imp._owners: deni, gogov, izomorphius Test_owners:
Analysis_reviewers: meddle Changelog:
Design_reviewers: Imp._reviewers: pap
Test_reviewers: Analysis_score: 4.5
Design_score: 3 Imp._score: 3
Test_score: 0

Description

Change History

comment:1 Changed 10 years ago by gogov

  • Owner set to gogov
  • Status changed from new to s1a_analysis_started

comment:2 Changed 10 years ago by deni

  • Status changed from s1a_analysis_started to s1b_analysis_finished

comment:3 Changed 10 years ago by meddle

  • Status changed from s1b_analysis_finished to s1c_analysis_ok
  • Design_score changed from 0 to 4.5
  • Analysis_reviewers set to meddle

I think this is good analysis for the both aspects and so I have no bad note and stuff :)

4.5p (10m)

comment:4 Changed 10 years ago by gogov

  • Status changed from s1c_analysis_ok to s2a_design_started

comment:5 Changed 10 years ago by gogov

  • Design_owners set to deni, gogov, izomorphius

comment:6 Changed 10 years ago by gogov

  • Status changed from s2a_design_started to s2b_design_finished

comment:7 Changed 10 years ago by gogov

  • Design_score changed from 4.5 to 0
  • Analysis_score changed from 0 to 4.5

comment:8 Changed 10 years ago by pap

  • Add Hours to Ticket set to 0
  • Design_score changed from 0 to 3
  • Status changed from s2b_design_finished to s2c_design_ok
  • Estimated Number of Hours set to 0.0
  • Billable? set
  • I am passing it although there are problems.
  • Algorithmic
    • No need of setUp/tearDown methods that just call the super method (QueryCacheTest and HasherTest)
    • This "assert count >= 0:"dasjldnasj" + count;" in the Hash class is not nice
    • No braces in ifs in the equals method of Hash class. If you use autogeneratted methods be sure to fix them.
    • Test for the immutability of Hash - arrays as parameters give possibility for rep exposure and this should be verified. This is important!!
    • Bad spacing in QueryCache - put a space between "if" keyword and its opening bracket.
    • It may be nice to put an @Immutable annotation to the immutable classes even if you don't suppose they'll be used in with pro-lib
    • You could use 3.5f instead of (float)3.5 - it is shorter (HasherTest)
    • Fake JavaDoc in HasherTest - I understand it is almost(or fully) meaningless but you could just write everywhere "initial value".
    • Plese remove the Thumbs.db file in modules/org.sophie2.base.scene/src/test/resources/distrib/Thumbs.db
    • Shouldn't the Sprite class have a @Immutable ?
    • I am not really sure but is it needed the cache member of the Sprite class to be publicly accessible? I think it may better be private or protected
    • Perhaps there is more
  • Hardware
    • Things like " catch (IOException e) {

e.printStackTrace();

}" in your code are not nice.

  • Obviusly you are missing some points like effects but I think they can be added in the next revision of the task
  • I still don't see the deployment resolved but as this is a more general issue for this revision you may have some wooden solution that covers only some platforms with soem long wierd instructions.
  • As we discussed this solution makes the scenes totally inextensible but I think we'd better have a fully working second solution and later on think how to split scene engines to reach extensibility.
  • Good luck and please do not make the perfect solution now(especially the hardware part). You can improve it the next week.

comment:9 Changed 10 years ago by deni

  • Owner changed from gogov to deni
  • Status changed from s2c_design_ok to s3a_implementation_started
  • Total Hours set to 0

comment:10 Changed 10 years ago by dido

  • Status changed from s3a_implementation_started to s1c_analysis_ok

There are major changes inside the design so we'll return it to an_ok state

comment:11 Changed 10 years ago by gogov

  • Owner changed from deni to gogov
  • Status changed from s1c_analysis_ok to s2a_design_started

comment:12 Changed 10 years ago by gogov

  • Status changed from s2a_design_started to s2b_design_finished

comment:13 Changed 10 years ago by gogov

  • Status changed from s2b_design_finished to s2c_design_ok

comment:14 Changed 10 years ago by gogov

  • Status changed from s2c_design_ok to s3a_implementation_started

comment:15 Changed 10 years ago by gogov

  • Status changed from s3a_implementation_started to s3b_implementation_finished

comment:16 Changed 10 years ago by gogov

  • Imp._owners set to deni, gogov, izomorphius

comment:17 Changed 10 years ago by pap

  • Status changed from s3b_implementation_finished to s3c_implementation_ok
  • Imp._score changed from 0 to 3
  • Imp._reviewers set to pap
  • I pass it especially when it is already in the branch.
  • Lots of commented out code (mainly clipping).
  • Unnecessary tearDown methods(ImageSpriteTest, BlendSpriteTest, )
  • Throw UnsupportedOperationException for methods that are not implmeneted/supported (applyTo in effects).
  • I find it a bit strange that you have a ColorEffect as a member in the ShadowEffect class instead of an ImmColor.
  • No JavaDoc on some of SceneEffect's methods.
  • Probably you know that but the methods that apply effects with the OpenGL library probably need aditional parameters :)
  • I think that it may be nice that immutable classes have an @Immutable annotation (actually I find this annotation so nice that it should be included in the JDK) - TilePos, *Effect.
  • I don't like "getInstance" as a name in the EmptySprite class. While I was reading its usage I thought it was something lika factroy rather than a singleton.
  • As Sprite is an @Immutable it is nice that its subclasses should have that annotation too
  • In BlendSprite the method createBlend could just be called just create or make. Same with ClipSprite.
  • I find the Tile class quite strange. First it has representation exposure. Second it uses a BufferedImage instead of ImmImage. And I don't understand "FOR INTERNAL USE ONLY! DO NOT MODIFY THE ARGUMENT!". It seems that the method is for internal use only :)
  • The holy Meddle(may his sacred light shine upon us and light our path) says it is nice to have a space between "if" and "(" (TextureRegistry). Same with "for" and "(".
  • I think that when you are throwing an exception as a result of catching another one it is nice to supply the cause in the constructor of the new exception (SceneUtil.readPixels).
  • I would be very glad if you correct most of these problems. You may wish to do this along with the next revision of this task or separately.

comment:18 Changed 9 years ago by deyan

  • Status changed from s3c_implementation_ok to closed
  • Resolution set to obsolete

Batch update from file query-obsoleted.csv

Note: See TracTickets for help on using tickets.